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

build: enable gn check for //electron:electron_lib #17100

Merged
merged 6 commits into from Mar 5, 2019

Conversation

Projects
None yet
4 participants
@deepak1556
Copy link
Member

deepak1556 commented Feb 23, 2019

Description of Change

  • Fix gn check errors for //electron
  • Add --gn-check lint option
  • Add check to ci

The headers included from some private targets have been suffixed with nogncheck, these targets are known to be built as a transitive dep, so lets avoid a visibility patch in chromium. Also this gives a good idea of private headers we depend on, lets try to minimize them.

Checklist

Release Notes

Notes: no-notes

@electron-cation electron-cation bot added new-pr 🌱 and removed new-pr 🌱 labels Feb 23, 2019

Show resolved Hide resolved atom/browser/api/atom_api_app.cc

@deepak1556 deepak1556 force-pushed the gn_check branch from 1ba30c7 to 5abd6ee Feb 26, 2019

@deepak1556 deepak1556 marked this pull request as ready for review Feb 26, 2019

@deepak1556 deepak1556 requested a review from as a code owner Feb 26, 2019

@deepak1556

This comment has been minimized.

Copy link
Member Author

deepak1556 commented Feb 26, 2019

@nornagon can you review this again for precommit hook changes.

@@ -102,7 +102,8 @@
],
"*.{gn,gni}": [
"python script/run-gn-format.py",
"git add"
"git add",
"node script/lint.js --gn-check --"

This comment has been minimized.

@nornagon

nornagon Feb 26, 2019

Contributor

I think this should go before the git add...? everything else has git add as the last item. but @MarshallOfSound is the one who knows for sure i think. can gn check change a file?

This comment has been minimized.

@deepak1556

deepak1556 Feb 27, 2019

Author Member

can gn check change a file?

Nope it doesn't, the errors needs to be fixed manually. Thats why I added it after git add

}, {
key: 'gn-check',
run: (opts) => {
const env = Object.assign({

This comment has been minimized.

@nornagon

nornagon Feb 26, 2019

Contributor

might as well go with

{
  CHROMIUM_BUILDTOOLS_PATH: path.resolve(SOURCE_ROOT, '..', 'buildtools'),
  DEPOT_TOOLS_WIN_TOOLCHAIN: '0',
  ...process.env
}

This comment has been minimized.

@nornagon

nornagon Mar 1, 2019

Contributor

still think spread > Object.assign :)

}, process.env)
// Users may not have depot_tools in PATH.
env.PATH = `${env.PATH}${path.delimiter}${DEPOT_TOOLS}`
const outDir = path.resolve(SOURCE_ROOT, '..', 'out', 'CheckTesting' + Date.now())

This comment has been minimized.

@nornagon

nornagon Feb 26, 2019

Contributor

It might make sense to do this in a tmpdir rather than in out? Or if not the OS tmpdir, then src/electron/.gn-lint-cache/... and add that to gitignore?

Also, we don't necessarily need to rerun gn gen from scratch every time... i think caching it and rerunning gn gen on the same dir every time would be fine.

env.PATH = `${env.PATH}${path.delimiter}${DEPOT_TOOLS}`
const outDir = path.resolve(SOURCE_ROOT, '..', 'out', 'CheckTesting' + Date.now())
const args = ['gen', outDir]
args.push('--args=\'import("//electron/build/args/testing.gn")\'')

This comment has been minimized.

@nornagon

nornagon Feb 26, 2019

Contributor

do the extra \' here do something in particular?

const args = ['gen', outDir]
args.push('--args=\'import("//electron/build/args/testing.gn")\'')

let result = childProcess.spawnSync('gn', args, { env, stdio: 'inherit', shell: true })

This comment has been minimized.

@nornagon

nornagon Feb 26, 2019

Contributor

can we do without shell: true?

if (opts.verbose) { console.log(`linting ${filenames.length} ${linter.key} ${filenames.length === 1 ? 'file' : 'files'}`) }
linter.run(opts, filenames)
if (linter.key === 'gn-check') {
linter.run(opts)

This comment has been minimized.

@nornagon

nornagon Feb 26, 2019

Contributor

we should only run gn check if a *.{gn,gni,h,c,cc,m,mm} file changes.

This comment has been minimized.

@deepak1556

deepak1556 Feb 27, 2019

Author Member

Adding this to lint-staged hook should take care of it.

This comment has been minimized.

@nornagon

nornagon Mar 1, 2019

Contributor

since this is running in CI now, this is moot.

@deepak1556 deepak1556 added the wip label Feb 27, 2019

@deepak1556 deepak1556 force-pushed the gn_check branch from 5abd6ee to 5e037d6 Feb 27, 2019

@deepak1556 deepak1556 requested a review from nornagon Feb 27, 2019

@deepak1556

This comment has been minimized.

Copy link
Member Author

deepak1556 commented Feb 27, 2019

gn gen takes considerable time that might be intrusive for a pre-commit hook even if its run on a cached dir. Moreover we need to test both testing and debug config, maybe we should just have this as a pre-push hook instead ?

@nornagon

This comment has been minimized.

Copy link
Contributor

nornagon commented Feb 27, 2019

Could we run it on CI instead?

@deepak1556

This comment has been minimized.

Copy link
Member Author

deepak1556 commented Feb 27, 2019

Yeah that would make things a lot easier, if I add them as a step in the testing and debug build CI runs, since this needs a complete source checkout and should be tested across all platforms. /cc @jkleinsc thoughts on this ?

@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented Feb 27, 2019

@deepak1556 How long would this take to run, and would the output / result change on different platforms?

@jkleinsc

This comment has been minimized.

Copy link
Contributor

jkleinsc commented Feb 27, 2019

@deepak1556 is there any reason to run it against multiple architectures or could we just run it on one?

@deepak1556

This comment has been minimized.

Copy link
Member Author

deepak1556 commented Feb 27, 2019

How long would this take to run

Header dependency check OK

real	0m10.266s
user	0m25.371s
sys	0m5.329s

would the output / result change on different platforms?

Yup only when you have platform specific includes and deps.

@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented Feb 28, 2019

Hm yeah that's probably a bit too long for pre-commit 🤔 I think letting the lint stage catch this on CircleCI would be enough

@deepak1556

This comment has been minimized.

Copy link
Member Author

deepak1556 commented Feb 28, 2019

I think letting the lint stage catch this on CircleCI would be enough

I don't think the lint ci has the complete src/ checkout, this check needs it. Any concern in adding this as a step in the build CI runs ? Just need the following variations, the cpu arch doesn't matter since we don't include deps based on that condition.

win-testing
win-debug
linux-testing
linux-debug
mac-testing
mac-debug
@nornagon

This comment has been minimized.

Copy link
Contributor

nornagon commented Feb 28, 2019

👍 on adding it to the regular CI.

@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented Feb 28, 2019

@deepak1556 I'm 👍 on adding it to those jobs you mentioned but only if it is the first step in those jobs. i.e. the check runs before we try to build Electron. Otherwise we might have to wait an hour just to find out about a lint failure 👍

@deepak1556 deepak1556 force-pushed the gn_check branch from 5e037d6 to 3ab801f Feb 28, 2019

@nornagon

This comment has been minimized.

Copy link
Contributor

nornagon commented Feb 28, 2019

Perhaps on circleci it would make sense to run it in parallel with the build? that way if lint fails, you still get to see the result of the tests.

@deepak1556 deepak1556 force-pushed the gn_check branch from 3ab801f to d0b0529 Feb 28, 2019

@deepak1556

This comment has been minimized.

Copy link
Member Author

deepak1556 commented Feb 28, 2019

I could use some help with fixing gn check errors on linux, thanks! will push fix for errors on windows shortly.

@deepak1556 deepak1556 force-pushed the gn_check branch from 934c962 to 6523767 Mar 1, 2019

@deepak1556 deepak1556 removed the wip label Mar 1, 2019

@deepak1556 deepak1556 force-pushed the gn_check branch from 6523767 to df93b48 Mar 1, 2019

}, {
key: 'gn-check',
run: (opts) => {
const env = Object.assign({

This comment has been minimized.

@nornagon

nornagon Mar 1, 2019

Contributor

still think spread > Object.assign :)

// no mode specified? run 'em all
if (!opts['c++'] && !opts.javascript && !opts.python && !opts.gn) {
opts['c++'] = opts.javascript = opts.python = opts.gn = true
// no mode specified? run 'em all excpet gn-check,

This comment has been minimized.

@nornagon

nornagon Mar 1, 2019

Contributor
Suggested change
// no mode specified? run 'em all excpet gn-check,
// no mode specified? run 'em all except gn-check,
if (opts.verbose) { console.log(`linting ${filenames.length} ${linter.key} ${filenames.length === 1 ? 'file' : 'files'}`) }
linter.run(opts, filenames)
if (linter.key === 'gn-check') {
linter.run(opts)

This comment has been minimized.

@nornagon

nornagon Mar 1, 2019

Contributor

since this is running in CI now, this is moot.

@@ -41,6 +41,32 @@ function spawnAndCheckExitCode (cmd, args, opts) {
if (status) process.exit(status)
}

function gnCheckConfig (config, env) {

This comment has been minimized.

@nornagon

nornagon Mar 1, 2019

Contributor

i'm inclined to drop this from the script now that it's just a simple gn check in CI.

This comment has been minimized.

@deepak1556

deepak1556 Mar 1, 2019

Author Member

Yup I would be in favor of that, wasn't sure if users wanted a simple one liner to replicate the failure they saw in ci.

This comment has been minimized.

@nornagon

nornagon Mar 1, 2019

Contributor

i think this one-liner will probably cause more problems than it solves. gn check out/Debug //electron:electron_lib is already a one-liner :)

@deepak1556 deepak1556 force-pushed the gn_check branch from df93b48 to 6f2c734 Mar 1, 2019

@deepak1556 deepak1556 requested a review from nornagon Mar 1, 2019

@deepak1556

This comment has been minimized.

Copy link
Member Author

deepak1556 commented Mar 4, 2019

@jkleinsc @MarshallOfSound can you review the ci config changes. Thanks!

@jkleinsc
Copy link
Contributor

jkleinsc left a comment

@deepak1556 given how quickly gn check runs, I don't think we should run it as a parallel job on CircleCI but should instead run it as part of our regular builds. Running as separate parallel jobs incurs about 5 minutes of extra compute per job and given the number of architectures we are using, it ends up being a significant amount of resources.

@jkleinsc

This comment has been minimized.

Copy link
Contributor

jkleinsc commented Mar 4, 2019

@nornagon since you suggested running gn check in parallel, I'm wondering if you still stand by that given how quickly gn check runs? We are incurring an extra 5 minutes of compute time to run a 15 second check.

@nornagon

This comment has been minimized.

Copy link
Contributor

nornagon commented Mar 4, 2019

the reason i suggested it being a parallel build is so that if the gn check fails you also get build/test results. it sucks when lint prevents your build from happening, especially when it takes 15+ minutes for that build to even start.

my desired solutions, from most to least desirable:

  1. have the check step run on the same checkout as the build/tests, but have it show up as a separate check from the build/test results in the GitHub UI
  2. have the check step run on the same checkout as the build/tests, but let the build keep going and only report failure at the end
  3. have the check step block the build

if we end up having to go with (3) it wouldn't be the worst thing, just a bit of a bummer.

@jkleinsc

This comment has been minimized.

Copy link
Contributor

jkleinsc commented Mar 4, 2019

I did not realize we were trying to build even if gn check failed. If that is the case, I'm fine with the PR as is.

@deepak1556 deepak1556 merged commit 7936237 into master Mar 5, 2019

8 checks passed

Semantic Pull Request ready to be squashed
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Mar 5, 2019

No Release Notes

@deepak1556 deepak1556 deleted the gn_check branch Mar 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.