Closed Bug 1315231 Opened 7 years ago Closed 7 years ago

Fix gyp build on Windows

Categories

(NSS :: Build, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ted, Assigned: ted)

References

Details

Attachments

(2 files, 5 obsolete files)

The gyp build got broken on Windows somewhere before landing. I had it working at some point. In any event, I have a patch.
Attached patch fix-nss-gyp-win.patch (obsolete) — Splinter Review
Assignee: nobody → ted
Status: NEW → ASSIGNED
Attachment #8807514 - Flags: review?(franziskuskiefer)
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)
Blocks: 1295937
Attached patch Fix gyp build on Windows (obsolete) — Splinter Review
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)
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?
(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.
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.
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)
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 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)
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.
Attached patch gyp-win-build.patch (obsolete) — Splinter Review
This patch changes build.sh to use detect_arch for detecting 64bit and passes nspr_ and nss_ dirs to gyp.
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?
Attached patch fix gyp build on windows (obsolete) — Splinter Review
This patch addresses your comments.
Attachment #8807687 - Attachment is obsolete: true
Attachment #8809135 - Flags: review?(franziskuskiefer)
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)
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 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)
(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.
Attachment #8809351 - Flags: review?(ttaubert) → review+
Attachment #8809143 - Flags: review?(franziskuskiefer) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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.
but this way it works :)
You need to log in before you can comment on or make changes to this bug.