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.

Sign up for GitHub

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

Jump to bottom

INF-629: Update common to get split builds #200

Merged
merged 3 commits into from
Nov 25, 2019
Merged

INF-629: Update common to get split builds #200

merged 3 commits into from
Nov 25, 2019

Conversation

nmattia
Copy link
Contributor

@nmattia nmattia commented Nov 22, 2019

Updates the common repo to have split builds:

  • shell build
  • format build
  • lint + doc build
  • debug build
  • release build

Includes the following commits:

commit b35ba23941c5f3b555eba33c144f2cf9e5036cfb
Author: Joachim Breitner [email protected]
Date: Wed Nov 20 11:57:28 2019 +0100

Do not use lib.inNixShell (#69)

* Do not use lib.inNixShell

this does not just mean “is the curren derivation the target of
`nix-shell`”, but it also turns true when this is a _dependency_ of
some `nix-shell` (e.g. Motoko’s). This is bad, because it changes
the derivation of `drun`, so although `drun` in in the nix cache, when
trying to enter `nix-shell`, it now depends on a _different_ `drun` (one
built with `-D warnings`), so our poor people have to watch rust compile
stuff.

Debuggig this was also tricky, and required help from the nix IRC
channel. They said that using `lib.inNixShell` in a derivation like this
is a bad idea.

So we now omit `-D warnings` simply for the `debug` and `shell` build,
where that makes more sense, and do not use `lib.inNixShell`.

commit b4e92e6010ad7caee863cb8e74b8c1426f92794a
Merge: d862d5d f19329c
Author: Nicolas Mattia [email protected]
Date: Thu Nov 14 18:16:18 2019 +0100

Merge pull request #67 from dfinity-lab/nm-always-touch

Touch rust files in preBuild

commit f19329c71b275d58f19109ac923110963bfbc2d3
Author: Nicolas Mattia [email protected]
Date: Thu Nov 14 18:15:09 2019 +0100

Run preBuild and postBuild in lint

commit 639866f9ec57523c1f4a53c14205841503f4f8ae
Author: Nicolas Mattia [email protected]
Date: Thu Nov 14 17:49:31 2019 +0100

s/preBuild/preCheck

commit 5dfef852592e35e88c86d96280584f4d631d3269
Author: Nicolas Mattia [email protected]
Date: Thu Nov 14 17:36:10 2019 +0100

Fix standalone test evaluation

commit 3df1411493bb6edad6e54664fcc819e28715ed31
Author: Nicolas Mattia [email protected]
Date: Thu Nov 14 17:21:00 2019 +0100

Touch rust files in preBuild

commit d862d5d1c492a2077364d3771645c72e0233ce93
Merge: 8387b5b 0367c94
Author: Nicolas Mattia [email protected]
Date: Thu Nov 14 16:51:43 2019 +0100

Merge pull request #66 from dfinity-lab/nm-fix-standalone-builder

Port standalone builder to new rust builder

commit 0367c948b70603b29b0f4ceff45df2c29045fd80
Author: Nicolas Mattia [email protected]
Date: Thu Nov 14 16:31:21 2019 +0100

Port standalone builder to new rust builder

Before this commit, building standalone rust on Linux leads to evaluation errors.

commit 8387b5bfd0cd8df0acccdca53feb1c38010eecd3
Merge: 68ddd96 bc868d9
Author: Nicolas Mattia [email protected]
Date: Thu Nov 14 16:04:10 2019 +0100

Merge pull request #65 from dfinity-lab/nm-split-dfn-further

INF-484: INF-588: INF-581: Split rust jobs

commit bc868d9fc02230a5e6a0833833b9af26edcd7325
Author: Nicolas Mattia [email protected]
Date: Thu Nov 14 16:02:03 2019 +0100

Create empty directory for lint job

commit 68ddd96f7c6171cb5316b6ca51e137ff8d478e06
Merge: a9d886e d8b3e93
Author: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Date: Thu Nov 14 14:46:30 2019 +0000

Merge pull request #33 from dfinity-lab/update-niv

Update niv

commit d8b3e93d9bc8ad7ebe4eacea2bed47bd39c41f93
Author: Nikola Knezevic [email protected]
Date: Tue Oct 29 10:36:01 2019 +0100

Update niv

This is to help with a problem in other repos where we had an infinite
recursion due to setting up nixpkgs in nix-shell shebang line.

commit 765754201b3b899899af232a8c6f8fd4a01d7594
Author: Nicolas Mattia [email protected]
Date: Thu Nov 14 15:23:36 2019 +0100

Update naersk for faster builds

commit 0b7f62cad98e0a6374af1e2496ca86dd7868f485
Author: Nicolas Mattia [email protected]
Date: Thu Nov 14 15:22:52 2019 +0100

Make doc overridable

commit 5d40a3800d3eb2591330884784c2f3e6ac384494
Author: Nicolas Mattia [email protected]
Date: Thu Nov 14 15:22:38 2019 +0100

Remove unnecessary phase

commit 8385c76857644474749f454d032029b556133265
Author: Nicolas Mattia [email protected]
Date: Thu Nov 14 15:22:18 2019 +0100

Use releaseTools.aggregate to aggregate

commit 54e2e13973d1603500185936212bdb78ab4d837b
Author: Nicolas Mattia [email protected]
Date: Thu Nov 14 14:39:33 2019 +0100

INF-484: INF-588: INF-581: split rust jobs

This splits the rust jobs further, exporting a single interface for rust packages, exporting the following:

* `build`: the release build
* `debug`: the debug build
* `doc`: the built documentation
* `lint`: the lint checks
* `fileChecks`: extra file checks
* `fmt`: the formatting checks
* `shell`: a shell derivation

Moreover a target called `allChecks` is added, which builds all of the above.

commit 548f4e78dd896ac1cef990a20daf00fbb93a7655
Author: Nicolas Mattia [email protected]
Date: Thu Nov 14 14:38:10 2019 +0100

lib: inline moreutils

commit a9d886e463e3df12439c4e6888d26e4a6902e757
Author: Akhi Singhania [email protected]
Date: Thu Nov 14 12:30:49 2019 +0100

Add libfuse

commit 2f236bd89793c2816bed76832f996ec80173d620
Merge: f77e818 a73dfb9
Author: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Date: Wed Nov 13 16:02:53 2019 +0000

Merge pull request #63 from dfinity-lab/nm-split-checks

INF-581: INF-588: Split buildDfinityRustPackage jobs

commit a73dfb98a9a9e1d477a0f8ac05a0613a0f79e465
Author: Nicolas Mattia [email protected]
Date: Wed Nov 13 15:55:02 2019 +0100

INF-581: INF-588: Extract fmt check from rust workspace

commit 99b261dcc45c25d50b5721bd3d52747ebbb39303
Author: Nicolas Mattia [email protected]
Date: Wed Nov 13 13:25:20 2019 +0100

INF-581: INF-588: Extract file checks from rust workspace

commit f77e818eb07ca5731fe97004c4b7524132429128
Merge: bbf39c3 63bd2a5
Author: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Date: Wed Nov 13 10:45:50 2019 +0000

Merge pull request #62 from dfinity-lab/nm-refactor-build-dfn-rs-pkg

INF-581: INF-588: Refactor buildDfinityRustPackage

commit 63bd2a5d6ebfb2885a66bfde431de157057fee6f
Author: Nicolas Mattia [email protected]
Date: Wed Nov 13 10:55:45 2019 +0100

INF-581: INF-588: Refactor buildDfinityRustPackage

@nmattia nmattia requested a review from a team as a code owner November 22, 2019 15:58
paulyoung
paulyoung reviewed Nov 22, 2019
View reviewed changes
nix/rust-workspace.nix
$doc/nix-support/hydra-build-products
'';
})
# override all derivations and add DFX_ASSETS as an environment variable
Copy link
Contributor

Choose a reason for hiding this comment

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

We only need DFX_ASSETS for dfx. Do we need to change anything here so that we're not affecting our other Rust packages?

Copy link
Contributor

Choose a reason for hiding this comment

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

I realize that this is separate to the motivation for this PR, but the comment made me think about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed with Paul, we should only add DFX_ASSETS to the rust derivations.

eftychis
eftychis reviewed Nov 25, 2019
View reviewed changes
nix/rust-workspace.nix
{ lint = workspace.lint.overrideAttrs (oldAttrs: {
nativeBuildInputs = oldAttrs.nativeBuildInputs ++ [
cargo-graph
graphviz
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eftychis can you clarify? The SDK seem to use cargo graph, which requires graphviz.

eftychis
eftychis reviewed Nov 25, 2019
View reviewed changes
nix/rust-workspace.nix
if !lib.isDerivation drv then drv else
drv.overrideAttrs (_: {
DFX_ASSETS = runCommandNoCC "dfx-assets" {} ''
mkdir -p $out
Copy link
Contributor

Choose a reason for hiding this comment

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

can we get a bit more context here? I would rather not have to rebuild everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean? this is the same as before.

@nmattia
INF-629: Update common to get split builds
f247d12 
Updates the `common` repo to have split builds:

* shell build
* format build
* lint + doc build
* debug build
* release build

Includes the following commits:

commit b35ba23941c5f3b555eba33c144f2cf9e5036cfb
Author: Joachim Breitner <[email protected]>
Date:   Wed Nov 20 11:57:28 2019 +0100

    Do not use lib.inNixShell (#69)

    * Do not use lib.inNixShell

    this does not just mean “is the curren derivation the target of
    `nix-shell`”, but it also turns true when this is a _dependency_ of
    some `nix-shell` (e.g. Motoko’s). This is bad, because it changes
    the derivation of `drun`, so although `drun` in in the nix cache, when
    trying to enter `nix-shell`, it now depends on a _different_ `drun` (one
    built with `-D warnings`), so our poor people have to watch rust compile
    stuff.

    Debuggig this was also tricky, and required help from the nix IRC
    channel. They said that using `lib.inNixShell` in a derivation like this
    is a bad idea.

    So we now omit `-D warnings` simply for the `debug` and `shell` build,
    where that makes more sense, and do not use `lib.inNixShell`.

commit b4e92e6010ad7caee863cb8e74b8c1426f92794a
Merge: d862d5d f19329c
Author: Nicolas Mattia <[email protected]>
Date:   Thu Nov 14 18:16:18 2019 +0100

    Merge pull request #67 from dfinity-lab/nm-always-touch

    Touch rust files in preBuild

commit f19329c71b275d58f19109ac923110963bfbc2d3
Author: Nicolas Mattia <[email protected]>
Date:   Thu Nov 14 18:15:09 2019 +0100

    Run preBuild and postBuild in lint

commit 639866f9ec57523c1f4a53c14205841503f4f8ae
Author: Nicolas Mattia <[email protected]>
Date:   Thu Nov 14 17:49:31 2019 +0100

    s/preBuild/preCheck

commit 5dfef852592e35e88c86d96280584f4d631d3269
Author: Nicolas Mattia <[email protected]>
Date:   Thu Nov 14 17:36:10 2019 +0100

    Fix standalone test evaluation

commit 3df1411493bb6edad6e54664fcc819e28715ed31
Author: Nicolas Mattia <[email protected]>
Date:   Thu Nov 14 17:21:00 2019 +0100

    Touch rust files in preBuild

commit d862d5d1c492a2077364d3771645c72e0233ce93
Merge: 8387b5b 0367c94
Author: Nicolas Mattia <[email protected]>
Date:   Thu Nov 14 16:51:43 2019 +0100

    Merge pull request #66 from dfinity-lab/nm-fix-standalone-builder

    Port standalone builder to new rust builder

commit 0367c948b70603b29b0f4ceff45df2c29045fd80
Author: Nicolas Mattia <[email protected]>
Date:   Thu Nov 14 16:31:21 2019 +0100

    Port standalone builder to new rust builder

    Before this commit, building standalone rust on Linux leads to evaluation errors.

commit 8387b5bfd0cd8df0acccdca53feb1c38010eecd3
Merge: 68ddd96 bc868d9
Author: Nicolas Mattia <[email protected]>
Date:   Thu Nov 14 16:04:10 2019 +0100

    Merge pull request #65 from dfinity-lab/nm-split-dfn-further

    INF-484: INF-588: INF-581: Split rust jobs

commit bc868d9fc02230a5e6a0833833b9af26edcd7325
Author: Nicolas Mattia <[email protected]>
Date:   Thu Nov 14 16:02:03 2019 +0100

    Create empty directory for lint job

commit 68ddd96f7c6171cb5316b6ca51e137ff8d478e06
Merge: a9d886e d8b3e93
Author: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Date:   Thu Nov 14 14:46:30 2019 +0000

    Merge pull request #33 from dfinity-lab/update-niv

    Update niv

commit d8b3e93d9bc8ad7ebe4eacea2bed47bd39c41f93
Author: Nikola Knezevic <[email protected]>
Date:   Tue Oct 29 10:36:01 2019 +0100

    Update niv

    This is to help with a problem in other repos where we had an infinite
    recursion due to setting up nixpkgs in nix-shell shebang line.

commit 765754201b3b899899af232a8c6f8fd4a01d7594
Author: Nicolas Mattia <[email protected]>
Date:   Thu Nov 14 15:23:36 2019 +0100

    Update naersk for faster builds

commit 0b7f62cad98e0a6374af1e2496ca86dd7868f485
Author: Nicolas Mattia <[email protected]>
Date:   Thu Nov 14 15:22:52 2019 +0100

    Make doc overridable

commit 5d40a3800d3eb2591330884784c2f3e6ac384494
Author: Nicolas Mattia <[email protected]>
Date:   Thu Nov 14 15:22:38 2019 +0100

    Remove unnecessary phase

commit 8385c76857644474749f454d032029b556133265
Author: Nicolas Mattia <[email protected]>
Date:   Thu Nov 14 15:22:18 2019 +0100

    Use releaseTools.aggregate to aggregate

commit 54e2e13973d1603500185936212bdb78ab4d837b
Author: Nicolas Mattia <[email protected]>
Date:   Thu Nov 14 14:39:33 2019 +0100

    INF-484: INF-588: INF-581: split rust jobs

    This splits the rust jobs further, exporting a single interface for rust packages, exporting the following:

    * `build`: the release build
    * `debug`: the debug build
    * `doc`: the built documentation
    * `lint`: the lint checks
    * `fileChecks`: extra file checks
    * `fmt`: the formatting checks
    * `shell`: a shell derivation

    Moreover a target called `allChecks` is added, which builds all of the above.

commit 548f4e78dd896ac1cef990a20daf00fbb93a7655
Author: Nicolas Mattia <[email protected]>
Date:   Thu Nov 14 14:38:10 2019 +0100

    lib: inline moreutils

commit a9d886e463e3df12439c4e6888d26e4a6902e757
Author: Akhi Singhania <[email protected]>
Date:   Thu Nov 14 12:30:49 2019 +0100

    Add libfuse

commit 2f236bd89793c2816bed76832f996ec80173d620
Merge: f77e818 a73dfb9
Author: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Date:   Wed Nov 13 16:02:53 2019 +0000

    Merge pull request #63 from dfinity-lab/nm-split-checks

    INF-581: INF-588: Split buildDfinityRustPackage jobs

commit a73dfb98a9a9e1d477a0f8ac05a0613a0f79e465
Author: Nicolas Mattia <[email protected]>
Date:   Wed Nov 13 15:55:02 2019 +0100

    INF-581: INF-588: Extract fmt check from rust workspace

commit 99b261dcc45c25d50b5721bd3d52747ebbb39303
Author: Nicolas Mattia <[email protected]>
Date:   Wed Nov 13 13:25:20 2019 +0100

    INF-581: INF-588: Extract file checks from rust workspace

commit f77e818eb07ca5731fe97004c4b7524132429128
Merge: bbf39c3 63bd2a5
Author: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Date:   Wed Nov 13 10:45:50 2019 +0000

    Merge pull request #62 from dfinity-lab/nm-refactor-build-dfn-rs-pkg

    INF-581: INF-588: Refactor buildDfinityRustPackage

commit 63bd2a5d6ebfb2885a66bfde431de157057fee6f
Author: Nicolas Mattia <[email protected]>
Date:   Wed Nov 13 10:55:45 2019 +0100

    INF-581: INF-588: Refactor buildDfinityRustPackage
@nmattia nmattia force-pushed the nm-update-common-multi-build branch from d4749a8 to f247d12 Compare November 25, 2019 10:40
@nmattia nmattia force-pushed the nm-update-common-multi-build branch from bcbe822 to 5536aa6 Compare November 25, 2019 15:11
hansl
hansl approved these changes Nov 25, 2019
View reviewed changes
Copy link
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

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

Overall, LGTM, but the DFX_ASSETS seem overkill.

@mergify mergify bot merged commit 824280a into master Nov 25, 2019
@mergify mergify bot deleted the nm-update-common-multi-build branch November 25, 2019 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eftychis eftychis eftychis left review comments

@paulyoung paulyoung paulyoung left review comments

@hansl hansl hansl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nmattia @paulyoung @hansl @eftychis