Skip to content

refactor: the install.sh derivation#567

Merged
hansl merged 2 commits intomasterfrom
basvandijk/refactor-install
Apr 15, 2020
Merged

refactor: the install.sh derivation#567
hansl merged 2 commits intomasterfrom
basvandijk/refactor-install

Conversation

@basvandijk
Copy link
Contributor

@basvandijk basvandijk commented Apr 14, 2020

Commit Message

refactor: the install.sh derivation

  • refactors the install.sh Nix derivations to be easier to follow.

  • Remove the hack for shellcheck local lint rules and makes sure
    shellcheck and shfmt are correctly applied to the script.

  • Fixes linting errors that arose during the above step.

  • Slightly adapt publish.nix to copy manifest.json directly from a
    local path instead of from the install.sh derivation. There's no
    reason for these to be coupled like this.

  • Because of the above the s3cp utility had to be refactored. Before
    it took a pkg, path and dstDir parameter. Now this is simplified
    to a src and dst parameter.

  • Make install.sh executable so we can directly test it using:

$(nix-build ci/ci.nix -A install.x86_64-linux --no-link)
info: Executing DFINITY SDK install script, commit: 8561155f270281b0106f950a7318b4a11496177e
...

@basvandijk basvandijk marked this pull request as ready for review April 14, 2020 12:57
@basvandijk basvandijk requested a review from a team April 14, 2020 12:57
@basvandijk basvandijk requested a review from a team as a code owner April 14, 2020 12:57
Copy link
Contributor

@nmattia nmattia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand, where is shellcheck running now?

@@ -11,10 +11,5 @@ let
ci = import ./ci.nix { inherit src releaseVersion; };
in
if !doRelease then {} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we still need the doRelease?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, nothing changed in that regard.

We still only want to build (and thus run) the publish.dfx.x86_64-linux job when there was an actual release.

@basvandijk basvandijk force-pushed the basvandijk/remove-blobules-publishing-code branch from 17e25c8 to 8921189 Compare April 14, 2020 19:40
* refactors the `install.sh` Nix derivations to be easier to follow.

* Remove the hack for shellcheck local lint rules and makes sure
shellcheck and shfmt are correctly applied to the script.

* Fixes linting errors that arose during the above step.

* Slightly adapt `publish.nix` to copy `manifest.json` directly from a
local path instead of from the `install.sh` derivation. There's no
reason for these to be coupled like this.

* Because of the above the `s3cp` utility had to be refactored. Before
it took a `pkg`, `path` and `dstDir` parameter. Now this is simplified
to a `src` and `dst` parameter.
@basvandijk basvandijk force-pushed the basvandijk/refactor-install branch from 69bf4a3 to c659e01 Compare April 14, 2020 19:47
@basvandijk basvandijk changed the base branch from basvandijk/remove-blobules-publishing-code to master April 14, 2020 19:47
@basvandijk
Copy link
Contributor Author

I don't understand, where is shellcheck running now?

It's running here.

@basvandijk basvandijk requested a review from nmattia April 14, 2020 19:54
@hansl hansl merged commit 28cc81e into master Apr 15, 2020
@basvandijk
Copy link
Contributor Author

Thanks for the review @hansl.

BTW in other repos like dfinity and infra we have the policy that only the author merges his PRs (either directly or preferably via Mergify). I think the same policy would make sense for sdk. In this case I would have preferred to merge via Mergify's automerge-squash so that the commit gets the nice commit message as in the PR description.

@hansl
Copy link
Contributor

hansl commented Apr 15, 2020

It did (I made sure of that), I think you edited your PR description (adding the last example) after the facts, no?

@nmattia nmattia deleted the basvandijk/refactor-install branch April 16, 2020 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants