hermit-os / hermit-rs

Hermit for Rust.
Apache License 2.0
1.66k stars 86 forks source link

build: feature to download kernel into a tmp dir #601

Closed vapourismo closed 2 months ago

vapourismo commented 2 months ago

By enabling this flag you ask the hermit build script to download the kernel into a temporary directory. This is useful when trying to avoid conflicts with Cargo configuration that may arise when the kernel is built in a sub-directory of another crate's Cargo configuration.

Fixes #600

mkroening commented 2 months ago

I am very reluctant about this approach, since that would mean Hermit would be rebuilt from scratch on every build instead of doing incremental builds inside the target directory.

Does removing the offending fields in .cargo/config.toml not work in your case? Are you sure that huge build times would be worth the small inconvenience of manually specifying --target <TARGET> for you? You could add a small bash script, or Makefile, or use just to move your configuration there.

vapourismo commented 2 months ago

Does Cargo allow the hermit crate to retain build artefacts from previous builds? If hermit gets rebuilt then so is the kernel, right?

I haven't noticed anything being rebuilt excessively. However, in any case, the feature flag is optional.

Specifying --target manually is only a partial solution. The Cargo configuration has more uses beyond providing this flag. For example, correct conditional compilation in rust-analyzer.

mkroening commented 2 months ago

Does Cargo allow the hermit crate to retain build artefacts from previous builds? If hermit gets rebuilt then so is the kernel, right?

If nothing changes regarding Hermit, hermit's build.rs is not even called and the previous artifacts are used. If there are hermit-related changes, hermit's build.rs just calls hermit-kernel's xtask to build, which reuses the previously compiled artifacts.

With this change, every time hermit's build.rs is run, the kernel sources are redownloaded and recompiled, though with the same target directory. Currently, this change does not even work on rebuilds (I guess because the temporary directory is deleted immediately).

Modifying something outside OUT_DIR is not supported by Cargo and won't be supported by us. We will have to wait for a proper upstream fix for this, as described in https://github.com/hermit-os/hermit-rs/issues/600#issuecomment-2218300628.

I suggest you move the rust-analyzer config into a proper rust-analyzer config and use something external for everything else.

vapourismo commented 2 months ago

But does Cargo keep the OUT_DIR around? If the OUT_DIR is purged before a new call to build.rs then what you say is not applicable. Vice versa, if something changed about the parent crate (e.g. a configuration flag passed to the kernel compilation), isn't it unsafe to retain the previous build artefacts?

rust-analyzer doesn't support this type of configuration because one ought to use .cargo/config.toml for this. The configuration you linked only works if all packages in the workspace share the same compilation target which is not the case for us - this is exactly why rust-analyzer supports the usage of build.target in the config.tomls.

What is so bad about this feature flag? It's not enabled by default and it works. It provides a nice escape hatch for developers that need it. Without this feature flag one can't upgrade the Hermit kernel past 0.6.7 unless completely trashing the development experience.

mkroening commented 2 months ago

But does Cargo keep the OUT_DIR around?

Yes, it does.

Isn't it unsafe to retain the previous build artefacts?

No, it is totally safe, exactly how calling cargo build with different configuration without changing the target directory or calling cargo clean in between.

rust-analyzer doesn't support this type of configuration because one ought to use .cargo/config.toml for this.

While rust-analyzer will implicitly be affected by .cargo/config.toml as are all Cargo invocations, rust-analyzer itself is configured via the LSP, not via .cargo/config.toml.

What is so bad about this feature flag?

As explained before, this is unsupported by Cargo and completely prevents incremental compilation in addition to not even working as is (it throws a compilation error on feature changes), since the temporary directories are deleted after running build.rs.

I am happy to help you find a solution to your problem, but this approach is not the way forward.

A workaround that might work for you is specifying build.target-dir in .cargo/config.toml such that the Hermit kernel is downloaded and built somewhere unaffected by other .cargo/config.tomls. This would essentially move OUT_DIR wherever you like, but in a consistent way for Cargo.

I am asking you to keep civil and constructive about this issue.