Closed Bug 1668328 Opened 4 years ago Closed 4 years ago

gyp file reading does not deal with python path (which may be in $HOME) having spaces on Windows

Categories

(NSS :: Build, defect)

Desktop
Windows 10
defect

Tracking

(firefox-esr78 unaffected, firefox82 fixed, firefox83 fixed)

RESOLVED FIXED
Tracking Status
firefox-esr78 --- unaffected
firefox82 --- fixed
firefox83 --- fixed

People

(Reporter: Gijs, Assigned: rstewart)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(2 files)

STR:

  1. run ./mach mochitest path/to/test

ER:
no errors

AR:

Test configuration changed. Regenerating backend.
'c:\Users\First' is not recognized as an internal or external command,
operable program or batch file.

when home dir is c:\Users\First Last\. It's not clear what command generates this and I don't know how to get the test backend to be regenerated again short of clobbering, which I don't want to do.

I'm also seeing:

c:\Users\First Last\.mozbuild\_virtualenvs\mach_py2\Scripts\python.exe ./coreconf/detect_host_arch.py' returned exit status 1 while in path\to\srcdir\security\nss\nss.gyp. while trying to load path\to\srcdir\security\nss\nss.gyp

where I'm not clear if that is related or not.

This appears to be stopping me running tests, full stop. :-(

Type: task → defect

Full stack for the gyp thing is:

 File "testing/mochitest/mach_commands.py", line 326, in run_mochitest_general
    tests = mochitest.resolve_tests(test_paths, test_objects, cwd=self._mach_context.cwd)
  File "testing/mochitest/mach_commands.py", line 97, in resolve_tests
    tests = list(resolver.resolve_tests(paths=test_paths, cwd=cwd))
  File "testing/mozbase/moztest\moztest\resolve.py", line 857, in resolve_tests
    for test in self._resolve(**kwargs):
  File "testing/mozbase/moztest\moztest\resolve.py", line 638, in _resolve
    if (path in self.test_dirs or
  File "testing/mozbase/moztest\moztest\resolve.py", line 555, in test_dirs
    for test in self.tests:
  File "testing/mozbase/moztest\moztest\resolve.py", line 521, in tests
    for test in self.load_tests():
  File "testing/mozbase/moztest\moztest\resolve.py", line 402, in __call__
    gen_test_backend()
  File "python/mozbuild\mozbuild\gen_test_backend.py", line 50, in gen_test_backend
    backend.consume(data)
  File "python/mozbuild\mozbuild\backend\base.py", line 128, in consume
    for obj in objs:
  File "python/mozbuild\mozbuild\frontend\emitter.py", line 169, in emit
    for out in output:
  File "python/mozbuild\mozbuild\frontend\reader.py", line 890, in read_topsrcdir
    for gyp_context in g.results:
  File "python/mozbuild\mozbuild\frontend\gyp_reader.py", line 434, in results
    flat_list, targets, data = self._gyp_loader_future.result()
  File "third_party/python/futures\concurrent\futures\_base.py", line 398, in result
    return self.__get_result()
  File "third_party/python/futures\concurrent\futures\_base.py", line 357, in __get_result
    raise type(self._exception), self._exception, self._traceback
Keywords: regression
OS: Unspecified → Windows 10
Regressed by: 1662793
Hardware: Unspecified → Desktop
Summary: Something in setting up the test information backend not dealing with $HOME having spaces on Windows → gyp file reading does not deal with python path (which may be in $HOME) having spaces on Windows
Has Regression Range: --- → yes

Reverting the patch in bug 1662793 fixes this.

:rstewart, could you take a look please?

Flags: needinfo?(rstewart)

Looks like this gyp config file is not resilient to python being set to a path with a space in it.

I don't know anything about gyp unfortunately. Total shot in the dark, does this patch fix it? This strategy of adding quotes works in bash, anyway...

diff --git a/security/nss/coreconf/config.gypi b/security/nss/coreconf/config.gypi
index 696a204ad2a4..8cae4c48d808 100644
--- a/security/nss/coreconf/config.gypi
+++ b/security/nss/coreconf/config.gypi
@@ -12,7 +12,7 @@
         # chromium uses pymod_do_main, but gyp doesn't set a sensible
         # Python sys.path (gyp_chromium does).
         'python%': '<(python)',
-        'host_arch%': '<!(<(python) <(DEPTH)/coreconf/detect_host_arch.py)',
+        'host_arch%': '<!("<(python)" <(DEPTH)/coreconf/detect_host_arch.py)',
       },
       'python%': '<(python)',
       'host_arch%': '<(host_arch)',
@@ -66,12 +66,12 @@
           ],
         }],
         ['"<(GENERATOR)"=="ninja"', {
-          'cc_is_clang%': '<!(<(python) <(DEPTH)/coreconf/check_cc.py clang)',
+          'cc_is_clang%': '<!("<(python)" <(DEPTH)/coreconf/check_cc.py clang)',
         }, {
           'cc_is_clang%': '0',
         }],
         ['"<(GENERATOR)"=="ninja"', {
-          'cc_is_gcc%': '<!(<(python) <(DEPTH)/coreconf/check_cc.py gcc)',
+          'cc_is_gcc%': '<!("<(python)" <(DEPTH)/coreconf/check_cc.py gcc)',
         }, {
           'cc_is_gcc%': '0',
         }],
@@ -441,11 +441,11 @@
           }],
           [ 'disable_werror==0 and OS!="android" and OS!="win"', {
             'cflags': [
-              '<!@(<(python) <(DEPTH)/coreconf/werror.py)',
+              '<!@("<(python)" <(DEPTH)/coreconf/werror.py)',
             ],
             'xcode_settings': {
               'OTHER_CFLAGS': [
-                '<!@(<(python) <(DEPTH)/coreconf/werror.py)',
+                '<!@("<(python)" <(DEPTH)/coreconf/werror.py)',
               ],
             },
           }],
Flags: needinfo?(rstewart)

Can someone on NSS please take a look?

Assignee: nobody → nobody
Component: General → Build
Product: Firefox Build System → NSS
Version: unspecified → other

Kevin, can you look at this patch? It would be good to have in the next Nightly, else the bug which regressed it needs to be reverted.

Flags: needinfo?(kjacobs.bugzilla)

To be clear -- reverting the "regressing" patch is a non-starter. The state of the gyp-handling code before bug 1662793 was wrong, and we'll need to fix forward to avoid introducing more issues than reverting the patch would fix.

(In reply to Ricky Stewart from comment #4)

diff --git a/security/nss/coreconf/config.gypi b/security/nss/coreconf/config.gypi
-        'host_arch%': '<!(<(python) <(DEPTH)/coreconf/detect_host_arch.py)',
+        'host_arch%': '<!("<(python)" <(DEPTH)/coreconf/detect_host_arch.py)',

Wow, that is some syntax...

Applying this patch (ed.: all of it, not just the quoted bit) does appear to fix the issue.

bug 1662793 landed on 82 so I guess we'll want to uplift whatever fix we end up with.

I saw this in a drive-by and it looks completely fine. Does someone have a space in their python path?

If someone can provide a patch, preferably a phabricator one on the NSS repo, I'll land it. Kevin might beat me to it though as I'm off for a three day weekend.

Flags: needinfo?(rstewart)

I can write the patch, just give me a little bit to figure out how to do so :)

Flags: needinfo?(rstewart)
Assignee: nobody → rstewart

This fixes a breakage if the Python path happens to have a space in it.

Added kjacobs as the reviewer, someone else feel free to pick up the review as well if you're around.

Landed (in 3.58 for Fx83): https://hg.mozilla.org/projects/nss/rev/c7d3b214dd4199fc7ab6040a9e7ef14149ca2151

On its own, this doesn't seem to be worth spinning a point release of NSS for uplift to beta. I suggest we backport if/when a 3.57 point release happens, but please let me know if that's not workable.

Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(kjacobs.bugzilla)
Resolution: --- → FIXED
Target Milestone: --- → 3.58

Regarding Gijs' uplift request in https://bugzilla.mozilla.org/show_bug.cgi?id=1668328#c9 -

Since this is about running tests, we could cheat and uplift the fix to Beta without releasing a point release. It won't make it into the release artifacts that way, but in this case all that matters is for people working on beta (and soon mozilla-release), that mach test works. The released NSS won't even matter in that case.

Anyway, I think we can cheat this in mozilla-beta and be safe. It's heretical, sure, but I've walked through the fire before...

2020-10-05 Ricky Stewart <rstewart@mozilla.com>

* coreconf/config.gypi:
Bug 1668328 - Enclose Python paths in `coreconf/config.gypi` in
quotes

This fixes a breakage if the Python path happens to have a space in
it.

This is an uplift of d22eea8d776062ddb4028809d193a22267c4812e.

Comment on attachment 9180022 [details]
Bug 1668328 - Enclose Python paths in coreconf/config.gypi in quotes UPGRADE_NSS_RELEASE r=kjacobs,mt

Beta/Release Uplift Approval Request

  • User impact if declined: mach failures if the user's path to Python contains a whitespace character
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: n/a
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is only a buildsystem fix. If it doesn't work, it'll manifest as a failed build (which is already the case).
  • String changes made/needed: n/a
Attachment #9180022 - Flags: approval-mozilla-beta?

JC, can you also ensure that we uplift within NSS? I don't want this to be reverted if we are forced to uplift NSS changes.

Flags: needinfo?(jjones)
Flags: needinfo?(jjones)

Comment on attachment 9180022 [details]
Bug 1668328 - Enclose Python paths in coreconf/config.gypi in quotes UPGRADE_NSS_RELEASE r=kjacobs,mt

approved for 82.0b9

Attachment #9180022 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: