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

Minor follow-up/tweaks for CI (for working with templates and Windows test parallelization) #70

Closed
DeeDeeG opened this issue Jul 29, 2020 · 8 comments · Fixed by #78
Closed
Labels
bug Something isn't working CI

Comments

@DeeDeeG
Copy link
Member

DeeDeeG commented Jul 29, 2020

Noting things that need small updating or tweaks here and there after the big "templates" (#41 + #46) and "Windows test parallelization" (#50) PRs.

  • Job dependencies: The "Release Branch Build" and "Nightly Release" pipelines have an "upload artifacts" job. This used to depend on the "Windows" job, and still does, but it should actually now depend on the "Windows_RendererTests" job. (So it doesn't try to upload binaries for untested code).
    • Semi-related: Windows_RendererTests: This could possibly use a nice displayName. For example, for macOS there is macOS_tests --> displayName: macOS test (though this is vague, and I would prefer macOS package tests, so maybe the displayName on Windows can be Windows renderer tests.
  • Cache identifier: Most of the "bootstrap breaking" type changes we would make (Installing recent node and npm, installing recent compiler toolchains, installing apt packages, etc. etc.) have been moved from script/vsts/platforms/ [macos.yml, linux.yml, windows.yml] to script/vsts/platforms/templates/preparation.yml. We should add preparation.yml to the cache identifier.
  • Linux container comment: The Linux build no longer runs in a container. The comment about "this image is the host for the container" is not applicable to our fork anymore and can be deleted.
  • Slow build tools installation happens in Windows test jobs: We should not be upgrading npm and windows-build-tools in the Windows renderer test jobs. Use the npm bundled with Node. If I understand correctly, the windows-build-tools aren't even used when running the Renderer tests. Installing the environment takes about 9 minutes before tests even begin! This could speed up "run failed jobs" by quite a bit, both in time before we can press that button, and time it takes for re-runs to complete.
    • Can look into if such speedups are possible on macOS too, I suppose. But I don't think environment setup time is as egregious, and the package tests do actually need to build native code, so I think this optimization woul not be applicable the the macOS test jobs.
  • miscellaneous cleanup: These PRs have unearthed some odd things we and upstream do in CI that could be cleaned up. For example:
    • There is a mis-labeled "Install Windows Build Dependencies" script step. This actually just installs the node_modules for script/vsts. It is totally unused after being installed here. We should just delete this, it doesn't appear to do anything beside waste a couple minutes of time under CI on Windows.
      • (The script/vsts packages are needed for GetReleaseVersion and the "Upload Artifacts" steps of the "Release Branch Build" and "Nightly Release" pipelines. But the script/vsts/node_modules dependencies are already properly installed for those. We're just doing it an extra, totally unnecessary additional time on Windows.)
    • Okay, these are needed to run script/vsts/windows-run.js on Windows x86. We should condition this script to only run on Windows x86, and not run it on windows x64.

There may be more that I didn't noticed, or saw and forgot about. Will try to post them here (or in a PR!) once I see/remember them.

@aminya aminya added bug Something isn't working CI labels Jul 29, 2020
@DeeDeeG
Copy link
Member Author

DeeDeeG commented Aug 4, 2020

Not really a fixup after the templatization/parallelization PRs, but another miscellaneous tweak to do:

if we don't pass REPO_OWNER, MAIN_REPO and NIGHTLY_RELEASE_REPO in as env: variables in the yaml, we can default the values to atom, atom, and atom-nightly-releases respectively in the JavaScript files.

That way:

  • Upstream can merge our fork-friendly changes without having to set any variables in Azure Pipelines UI. Things "just work" for them, no changes needed.
  • We only have to set one variable (REPO_OWNER = atom-ide-community), not three.

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Aug 13, 2020

Some stuff identified to try from #78

  • Consolidate camelCase buildArch and ALL_CAPS_WITH_UNDERSCORES BUILD_ARCH into just one variable, preferably buildArch as it's easy on the eyes.
  • See if we can get away with not installing the script\vsts\node_modules dependencies on Windows x64, since they are never really used during a normal everything-went-well "bootstrap, build test" CI run of Windows x64.
    • It's still used to upload crash reports from CI, when something goes wrong, but we don't look at those crash reports. We can turn that off here, but we can't turn it off upstream unless upstream similarly doesn't care about the CI-generated crash reports.

@aminya
Copy link
Member

aminya commented Aug 13, 2020

Some stuff identified to try from #78

  • Consolidate camelCase buildArch and ALL_CAPS_WITH_UNDERSCORES BUILD_ARCH into just one variable, preferably buildArch as it's easy on the eyes.

BUILD_ARCH is defined in our Azure script. So that's my preferred way. But I did not remove those from the env map of Azure steps because it is nice to be able to see that which scripts depend on the arch.
https://github.com/atom-ide-community/atom/blob/6c2fed82f2d8b96bfd30b0620382eb9568cb58bb/script/vsts/platforms/templates/preparation.yml#L32-L38

  • See if we can get away with not installing the script\vsts\node_modules dependencies on Windows x64, since they are never really used during a normal everything-went-well "bootstrap, build test" CI run of Windows x64.

This should be a separate PR.

  • It's still used to upload crash reports from CI, when something goes wrong, but we don't look at those crash reports. We can turn that off here, but we can't turn it off upstream unless upstream similarly doesn't care about the CI-generated crash reports.

I look at those. They are useful in some cases.

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Aug 13, 2020

I'm honestly s bit confused why crash reports need to be uploaded to S3, perhaps because Azure Pipelines doesn't store them forever.

We certainly don't need to try to upload them to some S3 bucket. Uploading them to Azure Pipelines artifacts storage is already working ("stage crash reports" or some similar named step in CI).

As for buildArch, the snippet you just linked was exactly what I was mainly hoping to factor out. It's all redundant, and the CI is actually pretty minimal and readable if you take out some of these odd workarounds for things. Maybe I'll do a branch to prove the point that this can and should be simpler IMO.

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Aug 16, 2020

We do more preparation than is necessary (we run the whole preparation.yml template) before making a Nightly release.

We can speed up by dropping the template from Nightly in the Release job... or do what upstream does and only install a pinned version of Node as preparation, no further preparation needed.

Example of upstream's Nightly pipeline preparing quickly with only one step (install Node): https://github.visualstudio.com/Atom/_build/results?buildId=57005&view=logs&j=8d802004-fbbb-5f17-b73e-f23de0c1dec8

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Aug 16, 2020

We install npm twice during the Windows Build job.

@aminya aminya linked a pull request Aug 17, 2020 that will close this issue
@aminya aminya closed this as completed Aug 17, 2020
@aminya
Copy link
Member

aminya commented Aug 17, 2020

We do more preparation than is necessary (we run the whole preparation.yml template) before making a Nightly release.

We can speed up by dropping the template from Nightly in the Release job... or do what upstream does and only install a pinned version of Node as preparation, no further preparation needed.

Example of upstream's Nightly pipeline preparing quickly with only one step (install Node): github.visualstudio.com/Atom/_build/results?buildId=57005&view=logs&j=8d802004-fbbb-5f17-b73e-f23de0c1dec8

The nightly build is not performance-critical. When I made the templates I was aware of the extra steps that happen nightly. However, easier maintenance is more important than shaving 1min from a CI job that runs once in a while.

@aminya
Copy link
Member

aminya commented Aug 17, 2020

We install npm twice during the Windows Build job.

These should be fixed in #44

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants