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

Generate LICENSE files in create_dist #8560

Merged
merged 1 commit into from
Mar 10, 2020

Conversation

fmarier
Copy link
Member

@fmarier fmarier commented Mar 6, 2020

This effectively re-enables the LICENSE file creation and fixes #8618.

Submitter Checklist:

Test Plan:

  1. Go into chrome://credits.
  2. Ensure that "Background images" is present and that its licensing info can be expanded.
  3. Ensure that "Brave Ad Block" is present and that its licensing info can be expanded.

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.

@fmarier fmarier requested review from bridiver and bsclifton March 6, 2020 02:09
@fmarier fmarier self-assigned this Mar 6, 2020
@fmarier fmarier added this to the 1.7.x - Nightly milestone Mar 6, 2020
@petemill
Copy link
Member

petemill commented Mar 6, 2020

@fmarier how long does this step take? Concerned about affecting incremental build time. Perhaps we could just make sure that CI runs it for all PRs? Isn't it already?

@NejcZdovc
Copy link
Contributor

was this just an oversight when merging so CI failed, but was merged anyway? If that is the case I think this step is not needed

@fmarier
Copy link
Member Author

fmarier commented Mar 6, 2020

how long does this step take?

On my laptop, it takes 4 ms as measured by:

--- a/lib/build.js
+++ b/lib/build.js
@@ -94,7 +94,10 @@ const build = (buildConfig = config.defaultBuildConfig, options) => {
   touchOverriddenFiles()
   touchOverriddenVectorIconFiles()
   util.updateBranding()
+  const t0 = new Date().getTime();
   licensing.updateLicenses()
+  const t1 = new Date().getTime();
+  console.log('Updating licenses took ' + (t1-t0) + ' ms')

@petemill
Copy link
Member

petemill commented Mar 6, 2020

@fmarier lol ok carry on then!

lib/build.js Outdated
if (buildConfig === 'Release') {
licensing.updateLicenses()
}
licensing.updateLicenses()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like this should go in create_dist with no guard and then it should run for all PRs and that seems like a more appropriate place for it anyway

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was aiming to make this fail as early as possible (i.e. anytime a developer builds) so that a simple mistake can't get accidentally get merged to master and break CI builds.

Given the 4 ms overhead, that seems like a reasonable trade-off?

@fmarier fmarier force-pushed the francois-run-license-generator-in-debug branch from 35232de to 8da1669 Compare March 9, 2020 21:07
@fmarier fmarier changed the title Revert "Only generate LICENSE files in Release builds" Generate LICENSE files in create_dist Mar 9, 2020
@fmarier fmarier requested a review from bridiver March 9, 2020 21:07
@fmarier
Copy link
Member Author

fmarier commented Mar 9, 2020

Changing the approach based on @bridiver 's comments that this would set a bad precedent for other (more expensive) steps to be done in all builds.

Note that this revised PR does not help in any way to protect against mistakes like the one that we saw in brave/brave-core#4398. All it does is move the generation to a different part of the build step so that these checks continue to be done in CI regardless of whether CI does Debug or Release builds.

@fmarier
Copy link
Member Author

fmarier commented Mar 9, 2020

In order to re-regenerate the LICENSE files in a local build, now one has to run npm run create_dist.

bridiver
bridiver previously approved these changes Mar 9, 2020
@bsclifton
Copy link
Member

Blocked on resolving #8615

This will ensure that the licensing script runs for all CI builds
even if we change the build config for them.
@fmarier fmarier modified the milestones: 1.7.x - Beta, 1.8.x - Nightly Mar 10, 2020
@fmarier fmarier merged commit fb6ce21 into master Mar 10, 2020
@fmarier fmarier deleted the francois-run-license-generator-in-debug branch March 10, 2020 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need to re-enable generation of LICENSE files
5 participants