CI templates

[aminya]

Description of the change

• This uses Powershell 7 as a cross-platform shell for CI scripts:

  • PowerShell 7 allows using the same syntax and code across all the operating systems.
  • This simplifies CI scripts. For example, previously CMD was used in Windows, which is an old shell with limited functionality.

• This refactors CI scripts into separate files (templates):

  • It also allows using parallelization of the tests without copy-pasting the same code. For example:

    • In macOS, the same templates are reused in the test phase.
    • In Windows x86, the templates are used for parallelization of tests in Parallelize Tests #31

  • This makes it easier to manage CI code. For example, we can quickly compare the publish step for all operating systems without going through big files and trying to find the relevant steps.

Previously, CI scripts were written back in ~2017. At that time, Azure Templates had not been introduced yet, and because of that using PowerShell was not suitable yet. Now in 2020, we can benefit from these and improve the CI.

Verification

The CI passes

Release Notes

N/A

[aminya]

@DeeDeeG Because of this, we need to rebase other pull requests. So let's first merge this.

[DeeDeeG]

I hope we can combine the templates, especially macOS/Linux which are both UNIX-like and use mostly the same commands.

We can use conditional expressions looking at "$(Agent.OS)" to set these env vars, for example: https://github.com/atom-ide-community/atom/blob/91f302f5451780bf19d6d84436647297080b4ce9/script/vsts/platforms/templates/linux-bootstrap.yml#L7-L9

(Other than differing env vars, macos-bootstrap.yml and linux-bootstrap.yml are the same:

$ diff linux-bootstrap.yml macos-bootstrap.yml

7,9c7,8
<       CC: clang-5.0
<       CXX: clang++-5.0
<       npm_config_clang: 1
---
>       NPM_BIN_PATH: /usr/local/bin/npm
>       npm_config_build_from_source: true

)

If we use that conditional logic enough, I think we can combine all three OSes most of the time.

It would be good to streamline the CI by removing these env vars if not needed. We should track down the initial justification for including them (git blame is useful for this) and see if the specific env vars can/should be dropped. Don't go custom when defaults work. I can build Atom on a variety of machines, and I've never needed Clang to do it.

[DeeDeeG]

I'm personally not sure this is more readable. It's inconvenient to open multiple files and remember how they are referenced across several files.

[aminya]

The code for three operating systems is different enough (the scripts, paths, file names, etc).

If we want to combine these, we should use the same shell (Bash or PowerShell), and find a way to parameterize the paths, and file names.

[DeeDeeG]

This all seems good in the abstract... I am working on seeing why our CI doesn't tend to pass and I am finding all this diff that we already have from upstream hard to debug. Shouldn't we pause on this sort of stuff until we can get CI reliably passing?

And thank you for working on it, regardless.

[aminya]

This all seems good in the abstract... I am working on seeing why our CI doesn't tend to pass

This and #31 solve our CI problems.

[aminya]

This is ready to go 🚀

[DeeDeeG]

Thanks for all the work. I'll be taking a closer look some time at it so I understand what's changed. Apologies for all the grumbling about this. I may have gone overboard with that.

A request: I'd love it if we could squash this into fewer commits, with clearly separated changes per commit, and containing (to the extent possible) just the final version of this PR's changes. This would help ensure we don't get confused when trying to rebase on upstream_master in the future. I know I personally would appreciate it, anyhow.

Something like this:

• Commit 1: Add the new templates files (don't use them yet)
• Commit 2: Use the template files in platforms definitions and pipeline definitions

(Maybe split up "using templates in platform definitions" (Windows, macOS, Linux) from "using templates in pipeline definitions" (nightly, pull requests,release branches... But I'm not sure there would be a benefit.)

As mentioned above, this would be MUCH EASIER to rebase in the event of a merge conflict. Thank you for even considering!!! I volunteer to do it if you want! And if you'd rather not, I respect your decision. Just thought I'd say this sooner rather than later, so it's the easiest possible time to try and implement the squash/rewrite.

[DeeDeeG]

Some comments I have.

Impressed with the work this took. Happy to try it in our process and see if it works as well as it looks (it looks great!) Yes, it's different from before. But I've read it all now and I can understand it.

It is much easier to tell platform differences apart for repeated code now, because platform differences for repeated code lives within the same template. That's actually a big plus for readability.

Having to look at multiple files is lessened in its impact by the fact that these template files are supposed to "just work" and not need to be edited constantly, for one thing; And by the fact the the platform (Linux/macOS/Windows) yaml files are now much more skimmable. They can be read and comprehended more quickly.

I hope you don't mind my thoughts on this after it's already been merged. This is a post-merge review.

Thank you for looking and considering.

[DeeDeeG]

It feels like it could be good to have GetReleaseVersion in a template, too.

It's mostly identical everywhere it's used.

If you want I can try to do this.

[aminya]

As mentioned above, this would be MUCH EASIER to rebase in the event of a merge conflict.

I decided to not rebase our master anymore. See my comment here #28 (comment). It is harmful to our repository.

[aminya]

It feels like it could be good to have GetReleaseVersion in a template, too.

It's mostly identical everywhere it's used.

If you want I can try to do this.

We can do that. We run the following two times in each script.

      - script: |
          cd script\vsts
          npm install
        env:
          GITHUB_TOKEN: $(GITHUB_TOKEN)
        displayName: npm install

So if you can make a template that includes both the following steps and have a condition to not run the get-release-version step, we can do that.

      - script: |
          cd script\vsts
          npm install
        env:
          GITHUB_TOKEN: $(GITHUB_TOKEN)
        displayName: npm install

      - script: node script\vsts\get-release-version.js
        name: Version
        env:
          REPO_OWNER: $(REPO_OWNER)
          NIGHTLY_RELEASE_REPO: $(NIGHTLY_RELEASE_REPO)

You should use my $esc technique in build.yml for passing --nightly to the script on the nightly yml.

Please make your pull request to this branch, so I can upstream it as part of the PR.

[DeeDeeG]

This indentation makes condition and env variable. Bootstrapping always runs.

We should indent it two spaces less, toward the left.

(We should change this on master now, I guess.)

[aminya]

Thanks for catching this. I will fix it!

[DeeDeeG]

I can post updates to this branch and then open another PR from this CI_templates branch, and you can merge the new commits into master with a traditional merge commit, no rebasing needed. Sounds good?

(Then everything is in one place on this branch when you go to make a PR to upstream.)

Edit: We'll figure something out.

[aminya]

I will use git cherry pick to bring your commits to master.