Closed
Bug 1315231
Opened 7 years ago
Closed 7 years ago
Fix gyp build on Windows
Categories
(NSS :: Build, defect)
NSS
Build
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ted, Assigned: ted)
References
Details
Attachments
(2 files, 5 obsolete files)
12.63 KB,
patch
|
franziskus
:
review+
|
Details | Diff | Splinter Review |
2.29 KB,
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
The gyp build got broken on Windows somewhere before landing. I had it working at some point. In any event, I have a patch.
Assignee | ||
Comment 1•7 years ago
|
||
Comment 2•7 years ago
|
||
Comment on attachment 8807514 [details] [diff] [review] fix-nss-gyp-win.patch Review of attachment 8807514 [details] [diff] [review]: ----------------------------------------------------------------- shlibsign.py needs to set PATH for windows to work. Otherwise this looks fine.
Attachment #8807514 -
Flags: review?(franziskuskiefer)
Assignee | ||
Comment 3•7 years ago
|
||
I needed to tweak shlibsign.py a little bit more than that--it turns out it didn't handle the case where nss_dist_dir and nspr_lib_dir were different.
Attachment #8807514 -
Attachment is obsolete: true
Attachment #8807687 -
Flags: review?(franziskuskiefer)
Comment 4•7 years ago
|
||
I have to ask, why not make Foo and Foo_x64 different on other platforms? Isn't that the better fix? Or is the expectation that you have to build a new ninja config in order to get a different target architecture?
Comment 5•7 years ago
|
||
(In reply to Martin Thomson [:mt:] from comment #4) > I have to ask, why not make Foo and Foo_x64 different on other platforms? > Isn't that the better fix? Or is the expectation that you have to build a > new ninja config in order to get a different target architecture? Well, the problem is that gyp puts the nspr path into the ninja files such that you can't build 32 and 64 bit builds from the same ninja files. I think the only way to work around that would be to make our nspr paths intelligent enough to handle this. I don't think that's worth the effort. If a local 32bit build is really necessary, a clobber build takes a minute.
Comment 6•7 years ago
|
||
Isn't it just a matter of feeding different NSPR paths in based on target architecture? I mean, moving the initialization of all of the NSPR stuff into the target-specific code? You would need multiple NSPR builds though... I don't know, but I'm just concerned that this is a step away from being able to do that.
Comment 7•7 years ago
|
||
Ted, can we make this happen? We'd need a different NSPR path for 32 and 64 bit builds. If we set nspr_lib_dir and nspr_include_dir depending on ia32/x64, that should be possible (I think).
Flags: needinfo?(ted)
Assignee | ||
Comment 8•7 years ago
|
||
We could. That's a fair bit more work, and makes it harder for people to do a build if they only care about one architecture. I'm not sure the payoff is really worth it. I would get rid of the _x64 bits entirely if that didn't break gyp's ninja backend.
Flags: needinfo?(ted)
Comment 9•7 years ago
|
||
Comment on attachment 8807687 [details] [diff] [review] Fix gyp build on Windows Review of attachment 8807687 [details] [diff] [review]: ----------------------------------------------------------------- ::: coreconf/config.gypi @@ +401,5 @@ > }, > }, > }, > + # Common settings for release should go here. > + 'Release': { Please add a TODO comment for the 32/64 issue. ::: coreconf/shlibsign.py @@ +4,1 @@ > # License, v. 2.0. If a copy of the MPL was not distributed with this can you make this file use python2 env? (or python3 compatible if it's not already) @@ +9,5 @@ > import sys > > def main(): > + lib_paths = [] > + lib_paths = [os.path.normpath(a[2:]) for a in sys.argv[1:] if a.startswith('-L')] This doesn't work. There's no -L in the argument. I think getting the path from the provided libs as was done before is the better option here.
Attachment #8807687 -
Flags: review?(franziskuskiefer)
Assignee | ||
Comment 10•7 years ago
|
||
Did you miss this diff hunk? + 'action': ['<(python)', '<(DEPTH)/coreconf/shlibsign.py', '-L<(nspr_lib_dir)', '<@(_inputs)'] This does work, I tested it on a local Windows build.
Comment 11•7 years ago
|
||
This patch changes build.sh to use detect_arch for detecting 64bit and passes nspr_ and nss_ dirs to gyp.
Assignee | ||
Comment 12•7 years ago
|
||
As discussed on IRC, if the NSPR lib path and the nss_dist_dir are not the same the existing script doesn't work. I guess if we are going to assume that everyone is running gyp builds via build.sh then we can just assert that that is true and move on in life?
Assignee | ||
Comment 13•7 years ago
|
||
This patch addresses your comments.
Attachment #8807687 -
Attachment is obsolete: true
Attachment #8809135 -
Flags: review?(franziskuskiefer)
Assignee | ||
Comment 14•7 years ago
|
||
I had to tweak your patch to get my build to work right, but the combination of these two patches seems good.
Attachment #8808985 -
Attachment is obsolete: true
Attachment #8809139 -
Flags: review?(ttaubert)
Assignee | ||
Comment 15•7 years ago
|
||
Oops, I missed one thing--since the patch removes the _x64 configurations on non-Windows, we shouldn't use them in build.sh.
Attachment #8809135 -
Attachment is obsolete: true
Attachment #8809135 -
Flags: review?(franziskuskiefer)
Attachment #8809143 -
Flags: review?(franziskuskiefer)
Comment 16•7 years ago
|
||
Comment on attachment 8809139 [details] [diff] [review] Fix build.sh host architecture detection on Windows, explicitly pass nspr_{include,lib }_dir to gyp Review of attachment 8809139 [details] [diff] [review]: ----------------------------------------------------------------- ::: build.sh @@ +124,5 @@ > # Build NSPR. > make "${nspr_env[@]}" -C "$cwd" NSS_GYP_PREFIX=$obj_dir install_nspr > > # Run gyp. > + set -v -x Debugging leftovers? @@ +129,5 @@ > PKG_CONFIG_PATH="$obj_dir/lib/pkgconfig" \ > "${scanbuild[@]}" gyp -f ninja "${gyp_params[@]}" --depth="$cwd" \ > + --generator-output="." "$cwd/nss.gyp" \ > + -Dnspr_include_dir="$obj_dir/include/nspr/" \ > + -Dnspr_lib_dir="$obj_dir/lib" We should put this in gyp_params, like we do for -Dnss_dist_dir and -Dnss_dist_obj_dir.
Attachment #8809139 -
Flags: review?(ttaubert)
Assignee | ||
Comment 17•7 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #16) > > # Run gyp. > > + set -v -x > > Debugging leftovers? Yes, but I think it's worth leaving there so you can see the actual gyp invocation when running build.sh.
Assignee | ||
Comment 18•7 years ago
|
||
Attachment #8809139 -
Attachment is obsolete: true
Attachment #8809351 -
Flags: review?(ttaubert)
Updated•7 years ago
|
Attachment #8809351 -
Flags: review?(ttaubert) → review+
Updated•7 years ago
|
Attachment #8809143 -
Flags: review?(franziskuskiefer) → review+
Assignee | ||
Comment 19•7 years ago
|
||
https://hg.mozilla.org/projects/nss/rev/b494653e5be595f23348acc59cf9f90841a2c5a8 bug 1315231 - fix gyp build on windows. r=franziskus https://hg.mozilla.org/projects/nss/rev/218433fb345b658a934de20ebdbff99e608b4c24 bug 1315231 - Fix build.sh host architecture detection on Windows, explicitly pass nspr_{include,lib}_dir to gyp. r=ttaubert
Assignee | ||
Updated•7 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment 20•7 years ago
|
||
this needed another fix https://hg.mozilla.org/projects/nss/rev/697aaeda536948589fb6759e235f7e1486b524b3
Assignee | ||
Comment 21•7 years ago
|
||
I don't think this hunk should have been necessary: - 'nspr_libs%': ['nspr4.lib', 'plc4.lib', 'plds4.lib'], + 'nspr_libs%': ['libnspr4.lib', 'libplc4.lib', 'libplds4.lib'], The libs aren't actually named that way, and IIRC gyp just strips the lib prefix off at some point internally.
Comment 22•7 years ago
|
||
but this way it works :)
You need to log in
before you can comment on or make changes to this bug.
Description
•