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.

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

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

Closed

Conversation

vapourismo
Copy link

@vapourismo vapourismo commented Jul 9, 2024

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

@vapourismo vapourismo force-pushed the feature/avoid-cargo-config-clash branch 2 times, most recently from abf7257 to 59861d8 Compare July 9, 2024 18:40
@mkroening mkroening self-assigned this Jul 9, 2024
@mkroening mkroening self-requested a review July 9, 2024 19:16
@vapourismo vapourismo force-pushed the feature/avoid-cargo-config-clash branch from 59861d8 to e654c02 Compare July 9, 2024 21:04
@mkroening
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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 #600 (comment).

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

@mkroening mkroening closed this Jul 10, 2024
@vapourismo
Copy link
Author

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
Copy link
Member

mkroening commented Jul 11, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cargo configuration file messes with kernel compilation when using hermit crate
2 participants