Closed Bug 956597 Opened 6 years ago Closed 6 years ago

|make -C js/src source-package| is broken

Categories

(Firefox Build System :: General, defect)

All
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla29

People

(Reporter: Waldo, Assigned: glandium)

References

Details

Attachments

(3 files)

In investigating a potential bug affecting standalone, I discovered that the source-package target doesn't work.  It looks like there may be multiple issues here, because just fixing the first issue I encountered when trying to test my fix, uncovers another thing.  So there are almost certainly at least three separate issues here.

We should probably look into adding some sort of check: target in js/src that produces a release tarball and tests a simple SpiderMonkey-using program against it.  Otherwise, the build people are going to keep breaking us and we're not going to find out until someone happens upon the issue.

Patch for the first issue in line is forthcoming.
The change from build/virtualenv to build/virtualenv_packages.txt (plus a few other moves) broke the packaging script.

http://hg.mozilla.org/mozilla-central/rev/c678ea1db9c9

It looks to me like virtualenv_packages.txt is the only new file to package up.  The other files just moved around into directories that are already fully-recursively copied into the directory to be packaged.  If I try packaging up a source tarball with the |set -e| added but not the other half, I get an error.  With both added, packaging works.  So I think this is good.  (Running the configure script in the tarball fails, but those are the separate issues mentioned before.)
Attachment #8355921 - Flags: review?(gps)
Attachment #8355921 - Flags: review?(darkxst)
Attachment 8355921 [details] [diff] fixes issues introduced by bug 794506.

The next issue that needs resolving is a wholly-unguarded import of |mozwebidlcodegen| in python/mozbuild/mozbuild/backend/recursivemake.py, introduced by bug 928195.  We could add the relevant files to the SpiderMonkey package, maybe.  Or we could move the import into the single method in the file that needs it.  But it seems like an abstraction error for fully-generic files to be including DOM/WebIDL-specific stuff at all.  So I don't really know what the right fix is for that issue.  gps, any chance you could write a fix for that?  I can test if you need.
Blocks: 794506, 928195
Flags: needinfo?(gps)
Comment on attachment 8355921 [details] [diff] [review]
Properly package the virtualenv stuff

Review of attachment 8355921 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Sorry about breaking your build.

It would be really nice if we had automation coverage of all this. I'm all in favor of moving logic from buildbot and mozharness configs into the tree. If standalone SM builds want to do that, you'll have the support of the build module.
Attachment #8355921 - Flags: review?(gps) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Attachment 8355921 [details] [diff] fixes issues introduced by bug 794506.
> 
> The next issue that needs resolving is a wholly-unguarded import of
> |mozwebidlcodegen| in python/mozbuild/mozbuild/backend/recursivemake.py,
> introduced by bug 928195.  We could add the relevant files to the
> SpiderMonkey package, maybe.  Or we could move the import into the single
> method in the file that needs it.  But it seems like an abstraction error
> for fully-generic files to be including DOM/WebIDL-specific stuff at all. 
> So I don't really know what the right fix is for that issue.  gps, any
> chance you could write a fix for that?  I can test if you need.

I support shipping the WebIDL .py files over conditionally importing. The build system is moving towards a more unified and self-contained Python virtualenv environment. It's much easier to say "this is the environment we support - we support no others" than to have N variations that need consideration when maintaining code. The build module can provide a .py script for printing or packaging Python environment files if you don't want to replicate this logic in js/src. Just ask us for it.
Flags: needinfo?(gps)
(In reply to Gregory Szorc [:gps] from comment #4)
> The build module can provide a .py
> script for printing or packaging Python environment files if you don't want
> to replicate this logic in js/src. Just ask us for it.

Note that since the packaging rule requires configure to have run, we could probably just use the list from backend.RecursiveMakeBackend.pp. Although that doesn't cover e.g. tests.
FWIW, after mozwebidlcodegen, it's gyp that's missing... Maybe that's starting to be too much...
This works for me locally. I can understand gps's argument in comment 4, but on the other hand, this adds 7 MB of python code that will never be used to the source archive. I really think we'd be better off not with conditional importing. We probably don't even need to do that at the function level. We can just put the imports in try/except clauses.
Attachment #8358087 - Flags: review?(jwalden+bmo)
Attachment #8358087 - Flags: review?(gps)
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
This is the alternative fix, that avoids adding 7MB of unused python code.
Attachment #8358089 - Flags: review?(gps)
Comment on attachment 8358087 [details] [diff] [review]
Package mozwebidlcodegen and gyp in the js source archive

Review of attachment 8358087 [details] [diff] [review]:
-----------------------------------------------------------------

I am not really qualified to review this, except to say that if it works, it's good.  rs=me or something here, on the assumption you presumably tried things out here, I don't *really* know that this is the full, correct fix (although it looks like it probably is).  :-\

Presumably we could also just add a one-off mock of mozwebidlcodegen to the package, too, and that'd be good enough -- assuming none of the webidl code is invoked by the JS build system, which seems really likely.  Either setup seems pretty fugly, and equally workable.
Attachment #8358087 - Flags: review?(jwalden+bmo) → review+
Attachment #8358087 - Flags: review?(gps) → review+
Comment on attachment 8358089 [details] [diff] [review]
Don't fail to import gyp and mozwebidlcodegen

I prefer to keep the Python environment consistent if at all possible. I don't think source package distribution size is a concern at this juncture given the limited audience. The distributed package size doesn't increase by 7MB does it?
Attachment #8358089 - Flags: review?(gps) → review-
Also, we can always prune the source archive later if the overhead becomes a problem. Simplicity and consistency wins for the moment.
Whiteboard: [leave open]
Attachment #8355921 - Flags: review?(darkxst)
It seems a little odd to include 7MB of unused code in the source package, however I guess its fine, provided it doesnt end up in the final build.
https://hg.mozilla.org/mozilla-central/rev/d2d3bfbb75a6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.