Closed Bug 1416490 Opened 7 years ago Closed 7 years ago

Fix env arguments injected into js/src's configure on windows

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

Details

Attachments

(3 files)

Bug 1416490 - Check for a value passed to RUSTC_OPT_LEVEL rather than whether its value was a default.
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1416490 - Ensure environment variables that are configure options are passed from configure to old-configure.
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1416490 - Backed out changeset 6a412222f22e (bug 1320738) in favor of fixing the underlying issue.
59 bytes, text/x-review-board-request
glandium
: review+
Details 
See https://bugzilla.mozilla.org/show_bug.cgi?id=1320738#c19

This is definitely going to to bite us again unless we fix the underlying issue.
Summary: Fix env arguments injected from into js/src's configure on windows → Fix env arguments injected into js/src's configure on windows
I see the issue here. Running it through try to make sure fix doesn't have unwanted side effect...
Assignee: nobody → cmanchester
Come to think of it, the other thing we learned from bug 1320738 is that mozharness is not setting MOZ_PGO in the environment for devedition builds. This is probably not intended.
(In reply to Chris Manchester (:chmanchester) from comment #5)
> Come to think of it, the other thing we learned from bug 1320738 is that
> mozharness is not setting MOZ_PGO in the environment for devedition builds.
> This is probably not intended.

I'd be willing to bet that mozharness setting MOZ_PGO is the result of mozharness back in the day not using pgo-specific mozconfigs. This was before we had taskgraph and before PGO builds were a separate build type. We can likely stop setting MOZ_PGO from mozharness today.
Comment on attachment 8927680 [details]
Bug 1416490 - Check for a value passed to RUSTC_OPT_LEVEL rather than whether its value was a default.

https://reviewboard.mozilla.org/r/198958/#review204396
Attachment #8927680 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8927681 [details]
Bug 1416490 - Ensure environment variables that are configure options are passed from configure to old-configure.

https://reviewboard.mozilla.org/r/198960/#review204398

::: commit-message-89e42:1
(Diff revision 1)
> +Bug 1416490 - Ensure environment variables that are configure options are passed from configure to old-cofigure.

typo: old-configure

::: build/moz.configure/old.configure:360
(Diff revision 1)
> -    ret = subprocess.call(cmd)
> +    # Updating os.environ will usually modify the environment, but casting to
> +    # 'dict' negates this behavior, so we pass env explicitly.

actually, the code above is not meant to update os.environ, and was only meant to construct an environment to pass down to subprocess.call. I don't know how it ended up not being passed to subprocess.call, but that makes this comment not really useful.

::: build/moz.configure/old.configure:362
(Diff revision 1)
>  
>      log_size = os.path.getsize('config.log')
>  
> -    ret = subprocess.call(cmd)
> +    # Updating os.environ will usually modify the environment, but casting to
> +    # 'dict' negates this behavior, so we pass env explicitly.
> +    ret = subprocess.call(cmd, env=encode(dict(env)))

you shouldn't need to wrap env with dict. And you shouldn't need to encode if extra_env is empty. So you might as well encode extra_env when updating env, and just pass env=env here.
Attachment #8927681 - Flags: review?(mh+mozilla)
Comment on attachment 8927682 [details]
Bug 1416490 - Backed out changeset 6a412222f22e (bug 1320738) in favor of fixing the underlying issue.

https://reviewboard.mozilla.org/r/198962/#review204400
Attachment #8927682 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8927681 [details]
Bug 1416490 - Ensure environment variables that are configure options are passed from configure to old-configure.

https://reviewboard.mozilla.org/r/198960/#review204398

> you shouldn't need to wrap env with dict. And you shouldn't need to encode if extra_env is empty. So you might as well encode extra_env when updating env, and just pass env=env here.

We do need to encode `env` unconditionally, it seems somewhere our modifications from within moz.configure (where str = unicode) put unicode strings into os.environ.
Comment on attachment 8927681 [details]
Bug 1416490 - Ensure environment variables that are configure options are passed from configure to old-configure.

https://reviewboard.mozilla.org/r/198960/#review204628
Attachment #8927681 - Flags: review?(mh+mozilla) → review+
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/89af2bd84c00
Check for a value passed to RUSTC_OPT_LEVEL rather than whether its value was a default. r=glandium
https://hg.mozilla.org/integration/autoland/rev/0545f114fb5b
Ensure environment variables that are configure options are passed from configure to old-configure. r=glandium
https://hg.mozilla.org/integration/autoland/rev/2a2000298135
Backed out changeset 6a412222f22e (bug 1320738) in favor of fixing the underlying issue. r=glandium
https://hg.mozilla.org/mozilla-central/rev/89af2bd84c00
https://hg.mozilla.org/mozilla-central/rev/0545f114fb5b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: