Opened 2 years ago

Closed 2 years ago

#22106 closed enhancement (fixed)

setup.py: generate auto-generated files in setup()

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-7.6
Component: build Keywords:
Cc: embray, jdemeyer, fbissey Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: d9d6e54 (Commits) Commit: d9d6e54c6785097cbf6f6720bd46ff042bc2ca64
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

Follow-up on #21613 and #22094. Ticket for embray.

Change History (27)

comment:1 Changed 2 years ago by jdemeyer

  • Cc egourgoulhon removed
  • Description modified (diff)
  • Summary changed from Proper solution for "setup.py: run_autogen is ran too late" to setup.py: generate auto-generated files in setup()

comment:2 follow-up: Changed 2 years ago by embray

So basically you undid my work and then assigned me a ticket to redo it? Ok, thanks. <== (Sorry, that wasn't meant to be sarcastic; having the ticket is actually helpful).

Last edited 2 years ago by embray (previous) (diff)

comment:3 Changed 2 years ago by embray

  • Milestone changed from sage-7.6 to sage-7.5

Since I'm back from vacation now can you just let me fix this for 7.5?

comment:4 Changed 2 years ago by fbissey

  • Milestone changed from sage-7.5 to sage-7.6

I'll take full responsibility for the undoing. I don't know what is Volker timing for 7.5, I must say, but it was on my mind that we couldn't get 7.5 in that state. Blame me if it was put forward.

comment:5 Changed 2 years ago by fbissey

You may want to make it a blocker if you want to attract Volker's attention.

comment:6 Changed 2 years ago by embray

7.5 isn't released yet so I don't know why it can't wait.

"I don't know what is Volker timing for 7.5"

Yeah neither, it seems, does anybody. Maybe Sage should have some kind of release schedule...

comment:7 Changed 2 years ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/ticket-22106
  • Commit set to e82c80c6a860fcbbdcad88ab96fafed0e6c7d86a
  • Status changed from new to needs_review

Here's a workaround that deals with the issue without running code at the setup.py module level. It's related, somewhat, to #21682, which should now depend on this.

I still see this as something of a stop-gap solution. My longer-term plan is to implement something that allows Sage's individual sub-packages to provide configuration details on a per-package basis, to be collected by the main setup.py in a consistent manner (similar to, but not exactly the same as http://docs.astropy.org/en/stable/development/building.html#customizing-setup-build-for-subpackages). This will make it easier for individual packages to inject steps into the build process (sort of like sub-makes) at the appropriate build stages.

comment:8 Changed 2 years ago by mkoeppe

  • Status changed from needs_review to needs_work

There's no commit on the branch

comment:9 Changed 2 years ago by jdemeyer

Personally, I am really against pushing this to 7.5.

The current 7.5.rc1 is working fine, I would not want to risk to break stuff again. Especially build-system issues might take a while to be discovered.

I also don't see the urgency for this ticket. I agree that something needs to be done, but surely it can wait for Sage 7.6?

comment:10 in reply to: ↑ 2 Changed 2 years ago by jdemeyer

Replying to embray:

So basically you undid my work

To be fair, we only undid your work because it broke stuff.

comment:11 Changed 2 years ago by embray

Yeah, it's fine. I don't think it's urgent now.

comment:12 Changed 2 years ago by git

  • Commit changed from e82c80c6a860fcbbdcad88ab96fafed0e6c7d86a to 3bb5e73032c4da6f6d9851ee908efbc2eb239c2b

Branch pushed to git repo; I updated commit sha1. New commits:

3bb5e73Generate auto-generated sources as the earliest step in the 'build' command.

comment:13 Changed 2 years ago by embray

  • Status changed from needs_work to needs_review

New commits:

3bb5e73Generate auto-generated sources as the earliest step in the 'build' command.

comment:14 Changed 2 years ago by jdemeyer

  • Branch changed from u/embray/ticket-22106 to u/jdemeyer/ticket-22106

comment:15 Changed 2 years ago by jdemeyer

  • Commit changed from 3bb5e73032c4da6f6d9851ee908efbc2eb239c2b to 1286549a6568d268e6f97232221ab44cbd6ea2ea

I made some cosmetic changes.

One more thing: is it a problem if some packages appear more than once? Should we do something like

for pkg in autogen_all():
    if pkg not in self.distribution.packages:
        self.distribution.packages.append(pkg)

New commits:

1286549Cosmetic changes

comment:16 Changed 2 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to needs_info

comment:17 Changed 2 years ago by embray

I thought about that too, and even had that in an earlier version, but concluded that it's not a problem. If you find some case where it is a problem it could be changed.

comment:18 Changed 2 years ago by jdemeyer

Given that we both thought about it, you should add a small comment explaining that duplicated entries are potentially a problem but actually aren't a problem.

If this branch actually works (I have not tested it sufficiently yet), this can get positive_review from me.

comment:19 follow-up: Changed 2 years ago by embray

Well, you changed the branch to your branch so you're free to do that. It seems to me like a silly thing to comment on--at that point you might as well put the check in anyways just in case.

comment:20 in reply to: ↑ 19 Changed 2 years ago by jdemeyer

Replying to embray:

at that point you might as well put the check in anyways just in case.

OK, let's do that then.

comment:21 Changed 2 years ago by embray

Will you do it or shall I? I don't mind, it's just you already switched to your branch.

comment:22 Changed 2 years ago by jdemeyer

I'm currently doing other things. So if you have time, please go ahead.

comment:23 Changed 2 years ago by git

  • Commit changed from 1286549a6568d268e6f97232221ab44cbd6ea2ea to d9d6e54c6785097cbf6f6720bd46ff042bc2ca64

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

abf86eeGenerate auto-generated sources as the earliest step in the 'build' command.
d311e0eCosmetic changes
d9d6e54Only add packages which did not exist already

comment:24 Changed 2 years ago by jdemeyer

Rebased to 7.6 + added the changes discussed in 15

comment:25 Changed 2 years ago by jdemeyer

  • Status changed from needs_info to needs_review

comment:26 Changed 2 years ago by embray

  • Status changed from needs_review to positive_review

LGTM, thanks!

comment:27 Changed 2 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket-22106 to d9d6e54c6785097cbf6f6720bd46ff042bc2ca64
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.