Rejigger cargo manifest and config #280
Conversation
ermo
added
type: bug
ermo
force-pushed
the
rejigger-cargo-manifest-and-config
branch
2 times, most recently
from
120065a to
df10ea9
Compare
Cargo.toml
Outdated
| [profile.release] | ||
| lto = "thin" | ||
|
|
||
| [profile.packaging] | ||
| inherits = "release" | ||
| lto = true | ||
| codegen-units = 1 | ||
| opt-level = 3 | ||
| # allow packaging system to do it | ||
| strip = "none" | ||
| debug = true | ||
|
|
||
| # We want people who use the onboarding steps to get a nice compromise | ||
| # between fast compilation and fast runtime, but with checks in place | ||
| # and full backtraces. Hyperfine tests shows opt-level = 1 to be a good | ||
| # compromise between compile speed and runtime speed. | ||
| [profile.onboarding] | ||
| inherits = "dev" | ||
| opt-level = 1 | ||
| lto = "thin" | ||
| debug = true | ||
| strip = "none" |
There was a problem hiding this comment.
I don't think this is a good idea. Cargo.toml is for project or workspace configuration, while config.toml is for cargo configuration. The intent is that someone can override the config.toml at build time and get a different set of behavior.
boulder/build.rs
Outdated
| // only recompile when necessary | ||
| let top_level = PathBuf::from("..").canonicalize()?; | ||
| println!("cargo::rerun-if-changed={}/build.rs", top_level.display()); |
There was a problem hiding this comment.
These lines were only necessary since you were using a symlink for build.rs previously. Cargo already by default considers changes within the directory being built to invalidate cache results, so this is unnecessary.
.cargo/config.toml
Outdated
| # NB: os_release patterns need to be added to both target configs for this to | ||
| # work... | ||
| # | ||
| # The Solus toolchain supports zstd debug sections currently (Serpent doesn't) | ||
| [target.'cfg(any(os_release_id = "solus"))'] |
There was a problem hiding this comment.
I think it would be better to remove this and have build.rs emit rustflag configurations instead. I think your goal here is that these configuration options would take effect for compilation of all crates, but that's not actually going to work since project dependencies are built BEFORE execution of the build.rs of a given project/crate. Since link-args are only used at link-time putting them in build.rs is perfectly fine since boulder and moss are where the linking happens anyway.
ermo
force-pushed
the
rejigger-cargo-manifest-and-config
branch
2 times, most recently
from
f20065e to
5d320c1
Compare
|
Converting to draft as this clearly needs more work. |
ermo
force-pushed
the
rejigger-cargo-manifest-and-config
branch
2 times, most recently
from
8d07839 to
1ca802b
Compare
The goal is enable us to add distro-toolchain specific compile time options trivially, whilst still having reasonably conservative onboarding defaults, such that users can successfully run `just get-started` on a variety of distributions. By setting os_release_id="(...)" in build.rs in boulder/ and moss/, we can now add specific flags known to work on platforms where we control the toolchain build-time options. As of now, the logic is working well enough that compilation doesn't stop working if you're not on Solus. The current solution is meant as starting point for a potentially more refined future solution if necessary. Tested on fedora 39 and Solus. Signed-off-by: Rune Morling <[email protected]>
ermo
force-pushed
the
rejigger-cargo-manifest-and-config
branch
from
1ca802b to
159201e
Compare
|
|
With these changes,
just get-startedworks for me again on Fedora.It turns out that it's better to only rely on
[target.'cfg(...)']patterns for rustflags, as if there's a[build]section w/rustflags, it'll ignore the[target]sections entirely.This PR moves the build profiles and the target compiler settings to the
./.cargo/config.tomlfile.