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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update CI scripts to lint early and fail fast #19096

Merged
merged 6 commits into from Apr 4, 2019

Conversation

Projects
None yet
2 participants
@jasonrudolph
Copy link
Member

commented Apr 2, 2019

Issue or RFC Endorsed by Atom's Maintainers

This pull request closes #18940.

This pull request also helps us make progress toward #19006.

Description of the Change

This pull request enhances the CI builds to "fail fast" when a linting violation is present, so that developers can enjoy a faster feedback loop. (See #18940 for more discussion of the motivation.)

As @50Wliu mentions in #18940 (comment), script/lint depends on script/bootstrap. Prior to this pull request, script/bootstrap only ran as part of script/build. Because of that, we ran script/lint after script/build so that all of the dependencies for script/lint would be in place. However, script/build is one of the slowest parts of the CI build, so it would be nice if we didn't have to wait for script/build to complete in order to check for linting violations.

With the changes in this pull request, we now run script/bootstrap on its own early in the CI set-up, and that enables us to run script/lint before running script/build. 馃槄

Alternate Designs

Since script/bootstrap now runs earlier in the CI process, this pull request adds an option that allows you to tell script/build to skip bootstrapping. This isn't essential, but I think it's nice to have. As an alternative, instead of adding an option to script/build to allow you to skip bootstrapping, we could have script/build always bootstrap (just as it did prior to this pull request). This approach would result in less logic to maintain, but the unnecessary bootstrapping adds about 15 seconds to the build (as measured on a 2018 MacBook Pro). Since we're running script/lint before script/build, and since script/lint depends on script/bootstrap, we don't need to run script/bootstrap as part of script/build, so we can save some time by skipping it.

Possible Drawbacks

It introduces a small bit of additional conditional logic to script/build.

Verification Process

To verify these changes, I intentionally inserted a linting violation and confirmed that the builds failed fast. I then removed the linting violation and confirmed that all the builds passed. For more details, please see below.

Note: This pull request doesn't change anything about the AppVeyor build. In the AppVeyor build, script/lint and script/test both run as part of the test_script step here. To have script/lint run before script/build, I think we'd have to move script/build out of the build_script step into the test_script step, and that just seems weird. Given that we're hoping to retire AppVeyor before too long, I think we should leave this as is. Linting will still fail fast on Travis CI and Azure DevOps, so that will give developers the faster feedback loop that we're looking for.

Release Notes

N/A - This is not a user-facing change, so it probably shouldn't go into the release notes.


/fyi @Aerijo

jasonrudolph added some commits Apr 2, 2019

@jasonrudolph jasonrudolph self-assigned this Apr 2, 2019

- script/lint
- script/test
script: >
script/lint &&

This comment has been minimized.

Copy link
@jasonrudolph

jasonrudolph Apr 2, 2019

Author Member

If linting fails, we want the build to halt immediately. In order to make that happen Travis CI, we need to combine all of the commands together. /xref travis-ci/travis-ci#6188

@50Wliu

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

the unnecessary bootstrapping adds about 15 seconds to the build

Not necessarily related to this PR, but I noticed that we don't seem to be using npm ci on any of our CI instances. Maybe that would eliminate this difference or make it negligible?

Provide the BUILD_ARCH env var for bootstrapping Windows env on VSTS
`script/vsts/windows-run.js` checks the BUILD_ARCH env var, so let's 
make sure it's populated.
@jasonrudolph

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

Not necessarily related to this PR, but I noticed that we don't seem to be using npm ci on any of our CI instances.

Good point, @50Wliu. I could definitely see some benefit in investigating that in a follow-up PR. 馃檱

@jasonrudolph jasonrudolph marked this pull request as ready for review Apr 4, 2019

@jasonrudolph jasonrudolph merged commit b50818e into master Apr 4, 2019

2 checks passed

Atom Pull Requests #1.37.0-dev+37264 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@jasonrudolph jasonrudolph deleted the lint-early-and-fail-fast branch Apr 4, 2019

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