Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not iterate through GRDs for every single script and lib #7801

Open
wants to merge 1 commit into
base: master
from

Conversation

@petemill
Copy link
Member

petemill commented Jan 17, 2020

Anything which required Util was also in turn requiring l10nUtil, which performs computations in the module root and has some expectations as to the state of the source tree. This isn't ideal for scripts which run at any time. Whilst the perfect fix would be exporting this functionality as a function to be run on-demand and not in the module root, the fix in this commit reduces the time this occurs by removing the require from Util.js and therefore every other lib and script.

Fix #7800

Submitter Checklist:

Test Plan:

Build should start fine (first few moments is when updateBranding is run)

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions.

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.
Anything which required Util was also in turn requiring l10nUtil, which performs computations in the module root and has some expectations as to the state of the source tree. This isn't ideal for scripts which run at any time. Whilst the perfect fix would be exporting this functionality as a function to be run on-demand and not in the module root, the fix in this commit reduces the time this occurs by removing the require from Util.js and therefore every other lib and script
@petemill petemill requested review from yrliou, bsclifton, emerick and mkarolin Jan 17, 2020
@petemill petemill self-assigned this Jan 17, 2020
Copy link
Contributor

emerick left a comment

LGTM

Copy link
Member

bsclifton left a comment

LGTM too! 😄

@yrliou
yrliou approved these changes Jan 18, 2020
Copy link
Member

yrliou left a comment

LGTM, thanks!

@yrliou yrliou added this to the 1.5.x - Nightly milestone Jan 18, 2020
@petemill
Copy link
Member Author

petemill commented Jan 18, 2020

This failed during macOS test-install phase:

12:02:50  + '[' -z ']'
12:02:50  + BROWSER='Brave Browser Nightly'
12:02:50  + BUILD_TYPE=Release
12:02:50  + OUT_DIR=/Users/jenkins/jenkins/workspace/brave-browser-build-pr_PR-7801/src/out/Release
12:02:50  + '[' = true ']'
12:02:50  /Users/jenkins/jenkins/workspace/brave-browser-build-pr_PR-7801@tmp/durable-14ce177f/script.sh: line 10: [: =: unary operator expected
12:02:50  + hdiutil attach -nobrowse '/Users/jenkins/jenkins/workspace/brave-browser-build-pr_PR-7801/src/out/Release/Brave Browser Nightly.dmg'
12:02:50  hdiutil: attach failed - No such file or directory
@petemill
Copy link
Member Author

petemill commented Jan 18, 2020

Looks like CI timed out though most or all steps passed?!

@petemill petemill removed this from the 1.5.x - Release milestone Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.