build: add nix-shell and cargo at the root of SDK #41

hansl · 2019-09-20T23:33:03Z

This was driving me cray-cray.

paulyoung

I have reservations about moving Cargo-specific things to the root since not everything in the SDK will be written in Rust but maybe the workspace makes sense. In any case, we'd need to change how nix builds things and use the workspace instead.

The existing shell.nix should already be doing what we want. Is that not working for you?

hansl · 2019-09-21T22:27:49Z

My IDE (IntelliJ with the rust plugin) is not Nix aware (outside of highlighting and running) and without a Cargo at the root it fails to read the workspace.

This allows nix-shell from the root and then you can build rust (or javascript). We should support a workflow where nix-build or nix-shell at the root of the repo merges all possible shell states between the different projects. There are plenty of work (and PRs) that should cover all our languages.

paulyoung · 2019-09-23T18:26:04Z

We should support a workflow where nix-build or nix-shell at the root of the repo merges all possible shell states between the different projects.

The existing nix-shell already does that

This adds CI, formatting and clippy (linting) to the whole project's rust code, so some fixes in the lib/ folder are necessary. The tests still passes, nix-shell and nix-build works (with build outputting the right expected format).

hansl · 2019-09-24T16:10:45Z

@paulyoung please take another look. I'm going to rebase my other PR on this.

hansl · 2019-09-24T16:11:54Z

@chenyan-dfinity Please take a look at this as it will need to be merged (and probably conflict) with what you're doing. It should be the same code, just respecting the formatter and linter we use. Your other PR will have to pass CI now, but rebasing it on this one should run CI. So once this is in master you should probably merge or rebase the other PR.

chenyan-dfinity

LGTM. Thanks for doing this!

paulyoung

I would like to get feedback from @jwiegley and/or @basvandijk on the nix changes.

Maybe we should try to use https://github.com/dfinity-lab/common, but I'm not sure if that's ready yet.

paulyoung · 2019-09-24T17:16:54Z

.gitignore

@@ -4,6 +4,9 @@ result*
 build/
 target/

+# Nix-related directories.
+.cargo-home/


I remember setting an environment variable in the old package.nix file so that .cargo was used instead of .cargo-home.

I don't know enough about Cargo to know the difference between the two directories but at the time I made the change it seemed obvious that something was looking for things in .cargo-build that were in .cargo

Not sure if that is relevant; maybe we need to make sure that env var is still set? or unset it if we want to use .cargo-home?

.cargo is used by cargo for its configuration, I think? .cargo-home could be named anything that's not used. That's just what they use in the main repo. This is to avoid using a global cargo cache when using nix-shell then cargo build.

paulyoung · 2019-09-24T17:17:34Z

jobset.nix

@@ -2,5 +2,5 @@
 , crossSystem ? null
 , config ? {}
 }: {
-  inherit (import ./. { inherit system crossSystem config; }) dfinity-sdk;
+  inherit (import ./. { inherit system crossSystem config; }) rust-workspace;


We still want to build everything in the SDK on CI. This would only build the rust workspace.

Yes. I would expect you add another inherit in this file when adding JavaScript.

I think it makes more sense to say "build the SDK" (dfinity-sdk) and then update what's in the SDK.

paulyoung · 2019-09-24T17:18:43Z

default.nix

+let pkgs = import ./nix/nixpkgs.nix args; in {
+  inherit pkgs;
+
+  inherit (pkgs.dfinity-sdk.packages) rust-workspace;


It would be great if we could just build everything under packages

My knowledge of nix is limited enough that I don't have an answer for you, unfortunately. I can inherit an attribute, I can list attributes, but I don't know how to inherit all attributes (using a for loop of some sort?).

Does this work?

let pkgs = import ./nix/nixpkgs.nix args; in { inherit pkgs; } // pkgs.dfinity-sdk.packages

paulyoung · 2019-09-24T17:19:57Z

lib/dfx_derive/Cargo.toml

@@ -4,6 +4,7 @@ version = "0.1.0"
 authors = ["chenyan-dfinity <yan.chen@dfinity.org>"]

 [lib]
+name = "dfx_derive"


Off topic, but why is "dfx" part of the name in these? Isn't their use broader than that?

@chenyan-dfinity can answer this.

nix/overlays/dfinity-sdk.nix

paulyoung · 2019-09-24T17:30:57Z

nix/rust-shell.nix

+      export PATH="${rustc}/bin''${PATH:+:}$PATH"
+
+      # Set CARGO_HOME to minimize interaction with any environment outside nix
+      export CARGO_HOME=${pkgs.lib.dfinityRoot}/.cargo-home


Please see my comment about .cargo-home

paulyoung · 2019-09-24T17:33:20Z

nix/rust-workspace.nix

+    ## best to ignore it for now.
+    # postDoc = ''
+    #   cargo graph | dot -Tsvg > ./target/doc/dfx/cargo-graph.svg
+    # '';


How does the dfinity repo get around this?

Is this relevant for any licensing concerns? e.g. so it's easy to see where a crate is coming from if something shows up with an incompatible license

I have no idea (seriously). I looked into it and gave up after a while. They use the same versions of cargo and cargo-graph.

I modified my lock file to have the right fields and ran cargo graph manually and it gave me a single node.

nix/rust-workspace.nix

shell.nix

basvandijk · 2019-09-24T18:24:52Z

I'll look at this PR tomorrow. Note that I expect https://github.com/dfinity-lab/common and https://github.com/dfinity-lab/dfinity/pull/1266 will be ready some time tomorrow. I already started moving this sdk repo over to use common.

paulyoung · 2019-09-24T20:21:39Z

nix/overlays/dfinity-sdk.nix

-    inherit dfx;
+    packages = {
+        inherit rust-workspace;
+        rust-workspace-debug = rust-workspace.override (_: {


Right now this builds when running nix-build and on CI, in addition to a release build.

Should we move it to shells?

If I understand this right, moving this to shells would prevent it from running on CI. Correct?

Maybe not actually, since part of the motivation for shells is to build on CI. Let's figure it out later.

paulyoung · 2019-09-24T20:22:36Z

nix/rust-workspace.nix

+, release ? true # is it a "release" build, as opposed to "debug" ?
+, doClippy ? false
+, doFmt ? false
+, doDoc ? false


Don't we want doClippy ? true, doFmt ? true so they run by default? Right now they're not running on CI.

This is the way it's setup in the DFINITY repo. From a separate conversation:

the idea might be that there's no use running those twice on CI

and/or that you don't want to block a release of a product if it fails formatting (but PRs should not be accepted anyway)

and/or that some #[cfg()] might remove code that won't be checked in clippy (as it actually evaluates those) in release (although the same can be said the other way)

paulyoung

Approving so we can make some progress, and follow up with @basvandijk tomorrow

hansl · 2019-09-24T20:31:49Z

@basvandijk @jwiegley Let's move discussions and reviews of this PR to the JIRA ticket about unifying the build of everyone.

I think the general idea here should be that:

the repo directory structure does not separate per language, but per product / project
ideally, we should have a nix derivation per product / project that handles dependencies internally, and have a separate Cargo for the root for IDEs.
That setup is more complicated than we can maintain at the current state (we're in full feature development until at least 0.5.0), and as such we just going to duct-tape it until we have cycles to fix it properly. So for now, rust is its own Nix derivation (similar to the main repo IIRC), and JavaScript / Other products will be their own Nix derivation separately.
the Nix package at the root of the repo should be the union of all the different Nix derivations

hansl · 2019-09-24T21:27:58Z

Sorry for the fixups (and the upcoming force push). This PR found a lot of linting and formatting errors, and apparently clippy doesn't thoroughly go through all nodes of the AST (ie. fixing some linting errors might uncover more linting errors, even in unrelated functions).

@hansl

commit 7c5e71488df430b89de637a15b56814db196e752 Author: Islam El-Ashi <ielashi@users.noreply.github.com> Date: Sat Nov 7 00:30:05 2020 +0100 feat: DER-encode public key and principal ID when using Plain Authentication. (#48) BREAKING CHANGE If a developer relies on the inner work of SenderPubKey or manually instanciate Principal, the API changed. commit a254861ae4bfe608734c8c190e4033cfe39f047b Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri Nov 6 15:17:32 2020 -0800 chore(deps): bump node-fetch from 2.6.0 to 2.6.1 in /e2e/node (#49) Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1. - [Release notes](https://github.com/bitinn/node-fetch/releases) - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md) - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit 694849109bc28fea80119388504127e889b36d15 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri Nov 6 15:17:14 2020 -0800 chore(deps-dev): bump node-fetch from 2.6.0 to 2.6.1 in /packages/agent (#50) Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1. - [Release notes](https://github.com/bitinn/node-fetch/releases) - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md) - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit 88af47f79b5e9f3a5e08abbf0ce03b3140357fc0 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri Nov 6 15:16:53 2020 -0800 chore(deps-dev): bump node-fetch in /packages/bootstrap (#51) Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1. - [Release notes](https://github.com/bitinn/node-fetch/releases) - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md) - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit ba4b082e5795150176f7adad989c3613edceb85f Author: Hans <hans@larsen.online> Date: Thu Nov 5 12:49:59 2020 -0800 test: add ic-ref, ci e2e and lerna support to repo (#47) This refactors the repository to use lerna to manage its monorepo's packages. It also adds the e2e tests from the SDK repo that we previously had to remove. More e2e tests will be added later, but this unblocks working with DER and Identity work as we can now at least run some e2e tests now (instead of mocking the http connection). commit 8baa1cfc227d003c2806df1057b92009159df250 Author: Hans <hans@larsen.online> Date: Tue Nov 3 11:04:36 2020 -0800 feat: use anonymous principal by default in HttpAgent (#46) If the auth transform is missing and the principal is anonymous, use an anonymous auth transform. Otherwise, use the auth transform as normal. # BREAKING CHANGE Before, an auth transform would not be called if it was missing, now it results as an error if the principal is not anonymous. This was because just using the packet as is without putting it in an envelope was a bug and should never be sent to a replica anyway. Now this is explicit. commit 1a714e6c53f5e467112b964b71204b6d2eb545ef Author: Hans <hans@larsen.online> Date: Tue Nov 3 11:04:15 2020 -0800 feat: add a getPrincipal() method to Agents (#40) commit 324e856944adb7f79a2afa2cffbdb6d144b35ab4 Author: Hans <hans@larsen.online> Date: Tue Nov 3 08:49:44 2020 -0800 feat: add the canister ID to the window.ic object (#41) This can be useful to know the canister ID of the current frontend. commit 8680a2cf4994b45c6e00ffb0b5a12b379f5ff919 Author: Hans <hans@larsen.online> Date: Mon Nov 2 21:57:21 2020 -0800 feat: add a cache bust hash at the end of javascript output (#39) commit 9f2ed968b7cc9f4b854af5e23edca19d34124909 Author: Benjamin Goering <benjamin.goering@dfinity.org> Date: Tue Sep 22 15:28:49 2020 -0700 docs: polish docs/npm-publish.md per @hansl feedback (#37) via: pr feedback on dfinity/agent-js#36

@hansl

commit 19c28008db5fa1582da573088b17d0bdee9e8bc2 Author: Hans <hans@larsen.online> Date: Mon Nov 9 14:47:25 2020 -0800 revert: Remove lerna (PR #47) (#59) This removes lerna, but attempt to not remove the e2e tests and the new packages/files that were added after the last PR. commit 426a94f356bc4e27f50c8bec55071b82105adbd1 Author: Hans <hans@larsen.online> Date: Mon Nov 9 13:54:04 2020 -0800 feat: use anonymous for the first retrieve (#53) Also include some cleanup of the scripts that arent used, and add a diagram for the bootstrapping process. This does not affect the worker yet. Only the first retrieve of the index.js on localhost. BREAKING CHANGE If a canister has a retrieve() method that relies on Principal, the principal used will change. Co-authored-by: Benjamin Goering <benjamin.goering@dfinity.org> commit b795d8b34ce38dd35e1bcf2b80e33108cf74161d Author: Andrew Wylde <2126677+codelemur@users.noreply.github.com> Date: Mon Nov 9 09:41:13 2020 -0800 feat(idp): add identity-provider package to repository (#52) adds plain HTML/ts and test setup for project commit 7c5e71488df430b89de637a15b56814db196e752 Author: Islam El-Ashi <ielashi@users.noreply.github.com> Date: Sat Nov 7 00:30:05 2020 +0100 feat: DER-encode public key and principal ID when using Plain Authentication. (#48) BREAKING CHANGE If a developer relies on the inner work of SenderPubKey or manually instanciate Principal, the API changed. commit a254861ae4bfe608734c8c190e4033cfe39f047b Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri Nov 6 15:17:32 2020 -0800 chore(deps): bump node-fetch from 2.6.0 to 2.6.1 in /e2e/node (#49) Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1. - [Release notes](https://github.com/bitinn/node-fetch/releases) - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md) - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit 694849109bc28fea80119388504127e889b36d15 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri Nov 6 15:17:14 2020 -0800 chore(deps-dev): bump node-fetch from 2.6.0 to 2.6.1 in /packages/agent (#50) Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1. - [Release notes](https://github.com/bitinn/node-fetch/releases) - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md) - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit 88af47f79b5e9f3a5e08abbf0ce03b3140357fc0 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri Nov 6 15:16:53 2020 -0800 chore(deps-dev): bump node-fetch in /packages/bootstrap (#51) Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1. - [Release notes](https://github.com/bitinn/node-fetch/releases) - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md) - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit ba4b082e5795150176f7adad989c3613edceb85f Author: Hans <hans@larsen.online> Date: Thu Nov 5 12:49:59 2020 -0800 test: add ic-ref, ci e2e and lerna support to repo (#47) This refactors the repository to use lerna to manage its monorepo's packages. It also adds the e2e tests from the SDK repo that we previously had to remove. More e2e tests will be added later, but this unblocks working with DER and Identity work as we can now at least run some e2e tests now (instead of mocking the http connection). commit 8baa1cfc227d003c2806df1057b92009159df250 Author: Hans <hans@larsen.online> Date: Tue Nov 3 11:04:36 2020 -0800 feat: use anonymous principal by default in HttpAgent (#46) If the auth transform is missing and the principal is anonymous, use an anonymous auth transform. Otherwise, use the auth transform as normal. # BREAKING CHANGE Before, an auth transform would not be called if it was missing, now it results as an error if the principal is not anonymous. This was because just using the packet as is without putting it in an envelope was a bug and should never be sent to a replica anyway. Now this is explicit. commit 1a714e6c53f5e467112b964b71204b6d2eb545ef Author: Hans <hans@larsen.online> Date: Tue Nov 3 11:04:15 2020 -0800 feat: add a getPrincipal() method to Agents (#40) commit 324e856944adb7f79a2afa2cffbdb6d144b35ab4 Author: Hans <hans@larsen.online> Date: Tue Nov 3 08:49:44 2020 -0800 feat: add the canister ID to the window.ic object (#41) This can be useful to know the canister ID of the current frontend. commit 8680a2cf4994b45c6e00ffb0b5a12b379f5ff919 Author: Hans <hans@larsen.online> Date: Mon Nov 2 21:57:21 2020 -0800 feat: add a cache bust hash at the end of javascript output (#39) commit 9f2ed968b7cc9f4b854af5e23edca19d34124909 Author: Benjamin Goering <benjamin.goering@dfinity.org> Date: Tue Sep 22 15:28:49 2020 -0700 docs: polish docs/npm-publish.md per @hansl feedback (#37) via: pr feedback on dfinity/agent-js#36

hansl requested a review from paulyoung September 20, 2019 23:33

paulyoung reviewed Sep 21, 2019

View reviewed changes

hansl mentioned this pull request Sep 23, 2019

First Public Spec library draft and Request ID calculation in Rust #42

Merged

3 tasks

hansl force-pushed the hansl/root-cargo branch from e9c1222 to 9d33d61 Compare September 24, 2019 03:58

build: add nix-shell and cargo at the root of SDK

515f179

This adds CI, formatting and clippy (linting) to the whole project's rust code, so some fixes in the lib/ folder are necessary. The tests still passes, nix-shell and nix-build works (with build outputting the right expected format).

hansl force-pushed the hansl/root-cargo branch from 9d33d61 to 515f179 Compare September 24, 2019 05:10

chenyan-dfinity approved these changes Sep 24, 2019

View reviewed changes

paulyoung suggested changes Sep 24, 2019

View reviewed changes

fixup: comments from paul

f252860

hansl requested a review from paulyoung September 24, 2019 17:57

paulyoung requested review from basvandijk and jwiegley September 24, 2019 18:22

Hans Larsen added 2 commits September 24, 2019 11:44

Merge remote-tracking branch 'origin/master' into hansl/root-cargo

8ea95bf

fixup

d42d0da

paulyoung reviewed Sep 24, 2019

View reviewed changes

paulyoung approved these changes Sep 24, 2019

View reviewed changes

Hans Larsen added 3 commits September 24, 2019 13:37

fixup

c9da9c6

fixup

df5eba0

fixup

bbef7eb

hansl merged commit 28a0814 into master Sep 24, 2019

hansl deleted the hansl/root-cargo branch September 24, 2019 21:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: add nix-shell and cargo at the root of SDK #41

build: add nix-shell and cargo at the root of SDK #41

hansl commented Sep 20, 2019

paulyoung left a comment

hansl commented Sep 21, 2019

paulyoung commented Sep 23, 2019

hansl commented Sep 24, 2019

hansl commented Sep 24, 2019

chenyan-dfinity left a comment

paulyoung left a comment

paulyoung Sep 24, 2019

hansl Sep 24, 2019

paulyoung Sep 24, 2019

hansl Sep 24, 2019

paulyoung Sep 24, 2019

paulyoung Sep 24, 2019

hansl Sep 24, 2019

paulyoung Sep 24, 2019

paulyoung Sep 24, 2019

hansl Sep 24, 2019

paulyoung Sep 24, 2019

paulyoung Sep 24, 2019

paulyoung Sep 24, 2019

hansl Sep 24, 2019

basvandijk commented Sep 24, 2019

paulyoung Sep 24, 2019

hansl Sep 24, 2019

paulyoung Sep 24, 2019

paulyoung Sep 24, 2019

hansl Sep 24, 2019 •

edited

Loading

paulyoung left a comment

hansl commented Sep 24, 2019 •

edited

Loading

hansl commented Sep 24, 2019

build: add nix-shell and cargo at the root of SDK #41

build: add nix-shell and cargo at the root of SDK #41

Conversation

hansl commented Sep 20, 2019

paulyoung left a comment

Choose a reason for hiding this comment

hansl commented Sep 21, 2019

paulyoung commented Sep 23, 2019

hansl commented Sep 24, 2019

hansl commented Sep 24, 2019

chenyan-dfinity left a comment

Choose a reason for hiding this comment

paulyoung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basvandijk commented Sep 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hansl Sep 24, 2019 • edited Loading

Choose a reason for hiding this comment

paulyoung left a comment

Choose a reason for hiding this comment

hansl commented Sep 24, 2019 • edited Loading

hansl commented Sep 24, 2019

hansl Sep 24, 2019 •

edited

Loading

hansl commented Sep 24, 2019 •

edited

Loading