1315231 - Fix gyp build on Windows

Status: RESOLVED FIXED

(NSS :: Build, defect)

Description

[(not currently active) Ted Mielczarek]

The gyp build got broken on Windows somewhere before landing. I had it working at some point. In any event, I have a patch.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: fix-nss-gyp-win.patch

Comment 2

[Franziskus Kiefer [:franziskus]]

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.

Comment 3

[(not currently active) Ted Mielczarek]

Attachment: Fix gyp build on Windows

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.

Comment 4

[Martin Thomson [:mt:]]

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

[Franziskus Kiefer [:franziskus]]

(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

[Martin Thomson [:mt:]]

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

[Franziskus Kiefer [:franziskus]]

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).

Comment 8

[(not currently active) Ted Mielczarek]

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.

Comment 9

[Franziskus Kiefer [:franziskus]]

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.

Comment 10

[(not currently active) Ted Mielczarek]

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

[Franziskus Kiefer [:franziskus]]

Attachment: gyp-win-build.patch

This patch changes build.sh to use detect_arch for detecting 64bit and passes nspr_ and nss_ dirs to gyp.

Comment 12

[(not currently active) Ted Mielczarek]

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?

Comment 13

[(not currently active) Ted Mielczarek]

Attachment: fix gyp build on windows

This patch addresses your comments.

Comment 14

[(not currently active) Ted Mielczarek]

Attachment: Fix build.sh host architecture detection on Windows, explicitly pass nspr_{include,lib }_dir to gyp

I had to tweak your patch to get my build to work right, but the combination of these two patches seems good.

Comment 15

[(not currently active) Ted Mielczarek]

Attachment: fix gyp build on windows

Oops, I missed one thing--since the patch removes the _x64 configurations on non-Windows, we shouldn't use them in build.sh.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 17

[(not currently active) Ted Mielczarek]

(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.

Comment 18

[(not currently active) Ted Mielczarek]

Attachment: Fix build.sh host architecture detection on Windows, explicitly pass nspr_{include,lib }_dir to gyp

Comment 19

[(not currently active) Ted Mielczarek]

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

Comment 20

[Franziskus Kiefer [:franziskus]]

this needed another fix
https://hg.mozilla.org/projects/nss/rev/697aaeda536948589fb6759e235f7e1486b524b3

Comment 21

[(not currently active) Ted Mielczarek]

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

[Franziskus Kiefer [:franziskus]]

but this way it works :)