coq-community / templates

Templates for configuration files and scripts useful for maintaining Coq projects [maintainers=@palmskog,@Zimmi48]
The Unlicense
13 stars 8 forks source link

[Nix Action] Deploy to Cachix even for PR from forks. #100

Closed Zimmi48 closed 3 years ago

Zimmi48 commented 3 years ago

EDIT:

First part: Deploy to Cachix even for PR from forks.

Motivation

The goal is to avoid ever rebuilding the same thing twice in CI. When a PR is submitted from a fork, if it has to build a dependency, we don't want to waste this precious build time by discarding the build output. And if the PR is merged as is, it shouldn't be needed to rebuild anything once merged in the base branch (if it hasn't changed).

Solution

Use the pull_request_target event instead of the pull_request event to have access to secret environment variables in all cases. This is safe (no risk of linking the secret variables) even with untrusted code because nix-build builds the code in an isolated environment. The pull_request_target event ensures that the submitter of the pull request cannot change the GitHub workflow.

Note about modified workflows

When a GitHub workflow is modified, in order to test it, we do run it with the pull_request event. In this case, it is recommended submitted from a local branch (not from a fork) or to have an appropriate CACHIX_AUTH_TOKEN secret variable set in the fork.

Second part: Skip action/checkout@v2 step if there are no submodules. (kept for later)

Instead, we manually resolve the automatic merge commit of the PR head in the base branch created by GitHub and pass it to nix-build to test.

If there are submodules to check out, we fall back to the previous way of doing.

Motivation

As can be seen in this test workflow, we are more explicit on what we build. The main step shows nix-build https://coq.inria.fr/nix/toolbox --argstr job aac-tactics --arg override '{ coq = "master"; aac-tactics = "coq-community:10361884b6907430a4acea45c0288cf534714330"; }' instead of nix-build https://coq.inria.fr/nix/toolbox --argstr job aac-tactics --arg override '{ coq = "master"; aac-tactics = builtins.filterSource (path: _: baseNameOf path != ".git") ./.; }'. This means that we can reproduce the build locally (as long as the tested branches of the dependencies have not changed) by copy-pasting the command (we don't need to check out the correct ref before, and actually we don't even need to clone the repo first).

Zimmi48 commented 3 years ago

I have only tested this on PRs so far and I realize that there might be issues happening when running on branches, since github.event.number will be undefined.

Zimmi48 commented 3 years ago

Another issue that I hadn't yet tested for is the case where there is a merge conflict. In this case refs/pull/XXX/merge doesn't exist, but the workflow is still run (whereas with the pull_request event it wouldn't be run).

Zimmi48 commented 3 years ago

I will update this PR with the knowledge I acquired while working on https://github.com/coq-community/coq-nix-toolbox/pull/55.

Zimmi48 commented 3 years ago

Tested at coq-community/aac-tactics#86.

Zimmi48 commented 3 years ago

@palmskog I would recommend applying the changes from this PR to any repository using the Nix Action generated with the templates since before them there could be significant inefficiencies when running CI in a PR from a fork. I've opened https://github.com/coq-community/aac-tactics/pull/86 for testing (now ready to merge) but for the rest of the repositories, it should be quite straightforward.

palmskog commented 3 years ago

OK, thanks for the heads up, I will try to roll this out wherever I used the old simple Nix workflow.

Do you think this configuration approach should be fairly stable from now on, or is further tuning expected?

Zimmi48 commented 3 years ago

There's one more change to be discussed but which isn't absolutely required (to be found in the skip-checkout-step branch in this repo, I haven't opened a PR for it yet, it used to be the second commit in this PR).