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

More CI speed #1626

Merged
merged 3 commits into from
Sep 23, 2023
Merged

More CI speed #1626

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
75e66df
speed up nickelWasm
Radvendii Sep 22, 2023
df69391
build packages separately so all dependencies get cached
Radvendii Sep 22, 2023
f15f3a4
comment
Radvendii Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions flake.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,28 @@
# Build *just* the cargo dependencies, so we can reuse all of that work (e.g. via cachix) when running in CI
cargoArtifacts = craneLib.buildDepsOnly {
inherit pname src;
cargoExtraArgs = "${cargoBuildExtraArgs} --workspace --all-features";
cargoExtraArgs = "${cargoBuildExtraArgs} --all-features";
Copy link
Member

Choose a reason for hiding this comment

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

Should we keep all features here? Or keep it but make sure that the subsequent buildPackage also have all-features? From our last discussion I had the impression that what we want is that the features set used to build the packages should be - as much as possible - the same feature set as the one used in cargoArtifacts. In fact I think on the CI we should probably always enable all features by default (the best would be to test different feature sets, but if we have to chose one feature set, all-features is probably the one with the maximal covering)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only an issue if we have upstream dependencies whose feature sets depends on the set of features enabled on our crates. At the moment, we don't have any such situations, but in theory it could come up in the future. My impulse is to deal with it if/when it comes up, but the downside of that is that nobody will notice anything is wrong, CI will just silently start taking a little longer.

# If we build all the packages at once, feature unification takes
# over and we get libraries with different sets of features than
# we would get building them separately. Meaning that when we
# later build them separately, it won't hit the cache. So instead,
# we need to build each package separately when we are collecting
# dependencies.
cargoBuildCommand = "cargoWorkspace build";
cargoTestCommand = "cargoWorkspace test";
cargoCheckCommand = "cargoWorkspace check";
preBuild = ''
cargoWorkspace() {
command=$(shift)
for packageDir in $(${pkgs.yq}/bin/tomlq -r '.workspace.members[]' Cargo.toml); do
(
cd $packageDir
pwd
cargoWithProfile $command "$@"
Copy link
Member

Choose a reason for hiding this comment

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

Where's cargoWithProfile defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

somewhere in craneLib. it's the default value of cargo{Build,Test,Check}Command

)
done
}
'';
# pyo3 needs a Python interpreter in the build environment
# https://pyo3.rs/v0.17.3/building_and_distribution#configuring-the-python-version
buildInputs = [ pkgs.python3 ];
Expand Down Expand Up @@ -618,9 +639,11 @@
nickel-lang-cli
nickel-lang-core
rustfmt;
# An optimizing release build is long: eschew optimizations in checks by
# building a dev profile
nickelWasm = buildNickelWasm { profile = "dev"; };
# There's a tradeoff here: "release" build is in theory longer than
# "dev", but it hits the cache on dependencies so in practice it is
# shorter. Another option would be to compile a dev dependencies version
# of cargoArtifacts. But that almost doubles the cache space.
nickelWasm = buildNickelWasm { profile = "release"; };
inherit vscodeExtension;
pre-commit = pre-commit-builder { };
};
Expand Down