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

add description to build the kernel with docker #113

Closed
wants to merge 8 commits into from

Conversation

stlankes
Copy link
Contributor

No description provided.

@stlankes
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Oct 10, 2020
@bors
Copy link
Contributor

bors bot commented Oct 10, 2020

try

Build failed:

@stlankes
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Oct 11, 2020
@bors
Copy link
Contributor

bors bot commented Oct 11, 2020

Copy link
Contributor

@jbreitbart jbreitbart left a comment

Choose a reason for hiding this comment

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

I am not sure what you are trying to achieve. Are you just trying to pin the version of the nightly compiler that is used to build libhermit-rs? If that is the case than please use the rust-toolchain file and set it to a specific nightly version. I believe the syntax is nightly-YYYY-MM-DD to select the nightly version of a specific day.

Using docker for this task seems to be over-engineering the problem.

@stlankes
Copy link
Contributor Author

stlankes commented Oct 11, 2020

The docker container includes also patches, which are currently under review. Consequently, I am able to provide fixes, which are currently not part of the nightly compiler.

@jbreitbart
Copy link
Contributor

I see. I guess still disagree with what you are doing here. The master branch should compile with an official compiler release. If there is a change in the nightly compiler that results in libhermit-rs not compiling, we should pin the nightly compiler to a version that is actually working with our code. Changes to libhermit-rs that require yet unmerged changes to the Rust compiler/runtime should wait in a branch/fork until they are available to everyone.

@stlankes
Copy link
Contributor Author

stlankes commented Oct 12, 2020

What is the different? Using a specific container or using a specific nightly compiler? I would like to use hermit-os/hermit-rs#66. However, it is currently not part of rustup.

@jbreitbart
Copy link
Contributor

The difference is in the amount of dependencies that are required to build this repository. Docker + the image + the infrastructure to host the image are new dependencies. Just specifying the compiler does not change the dependencies to build the repository. What are the benefits of your approach that you think introducing these dependencies are really worth the trouble? Especially if you plan to use rustup later anyway.

@jschwe
Copy link
Contributor

jschwe commented Oct 12, 2020

Rustup doesn't seem to be following any regular release model, so I have no idea when the toml format for rust-toolchain, which would automatically install the toolchain and components, will be available to us. In the meantime, instead of requiring docker, it might be easier for new users if they are required to run something like the following script after every pull.

# Assuming rust-toolchain contains format nightly-YYYY-MM-DD
NIGHTLY_VERSION=$(cat rust-toolchain)
rustup toolchain install ${NIGHTLY_VERSION}
rustup +${NIGHTLY_VERSION} component add rust-src llvm-tools-preview

@jounathaen
Copy link
Member

@jschwe but you can put something like nightly-2020-03-15 in the rust-toolchain file since forever. If you run cargo build for example, it will invoke rustup automatically and download that toolchain.
You then still need to build rust-src but that is in the README anyway. (rustup component add rust-src) and cargo tells you to execute exactly these commands. This would be superfluous with the toml format.

I'd also agree with @jbreitbart that pinning the toolchain and providing a docker file are separate tasks which shouldn't be mixed.

What about updating the rust-toolchain file for every release to the last known working nightly but running the CI on the normal nightly by default?

@jschwe
Copy link
Contributor

jschwe commented Oct 12, 2020

@jounathaen Yes, exactly. What I wanted to express is that the new rust-toolchain toml format is more of a quality of life improvement, that reduces the manual steps a user needs to execute, but is not necessary for pinning a working nightly version. I would say that adding the missing components of the toolchain is probably easier for many users than building with docker.

[...] but running the CI on the normal nightly by default?

nightly could be an experimental (may fail) job. But I think the CI should generally still build with the pinned version, so that PRs don't get blocked simply because building with the current nightly fails.

@jbreitbart
Copy link
Contributor

[...] but running the CI on the normal nightly by default?

nightly could be an experimental (may fail) job. But I think the CI should generally still build with the pinned version, so that PRs don't get blocked simply because building with the current nightly fails.

Yes. It may make sense to have an action that runs say once a week and uses to latest nightly for all tests. That way we know if it is still working with nightly. In an ideal world that action would either create a pull request to update the rust-toolchain file or create an issue ... not sure if it is really worth the effort though.

@stlankes
Copy link
Contributor Author

I agree that the usage of docker isn't a good idea. However, I don't like to maintain rust-toolchain in libhermit-rs and rusty-hermit. In addition, I have also describe to install source code like rustup +nightly-2020-03-24 component add rust-src and to maintain it in our workflows.

In general it should work with latest nightly compiler. Consequently, we are talking about exceptions. Why should we not use docker for these exceptions.

@jbreitbart
Copy link
Contributor

I agree that the usage of docker isn't a good idea. However, I don't like to maintain rust-toolchain in libhermit-rs and rusty-hermit.

That is understood. But your docker container annoys not only you but every other possible user as well.

In addition, I have also describe to install source code like rustup +nightly-2020-03-24 component add rust-src and to maintain it in our workflows.

Have you tried that? Because that is not the behaviour I am seeing:

 ~/temp/libhermit-rs on  master *
 ᨂ cat -p rust-toolchain
nightly-2020-09-10

 ~/temp/libhermit-rs on  master *
 ᨂ cargo build
error: "/home/jbreitbart/.rustup/toolchains/nightly-2020-09-10-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/Cargo.lock" does not exist, unable to build with the standard library, try:
        rustup component add rust-src

 ~/temp/libhermit-rs on  master *
 ᨂ rustup component add rust-src
info: downloading component 'rust-src'
info: installing component 'rust-src'
info: Defaulting to 500.0 MiB unpack ram

 ~/temp/libhermit-rs on  master *
 ᨂ cargo build
  Downloaded hashbrown v0.8.2
  Downloaded 1 crate (75.8 KB) in 0.69s
...

There is not additional burden on maintaining the documentation.

In general it should work with latest nightly compiler. Consequently, we are talking about exceptions. Why should we not use docker for these exceptions.

Actually, that is a really awful goal. If someone reports a bug you have no idea which compiler is used and it is almost impossible to track down a bug if it is not obviously something within your code. You have to manage your dependencies, as annoying as it may be.

@stlankes
Copy link
Contributor Author

Have you tried that? Because that is not the behaviour I am seeing:

 ~/temp/libhermit-rs on  master *
 ᨂ cat -p rust-toolchain
nightly-2020-09-10

 ~/temp/libhermit-rs on  master *
 ᨂ cargo build
error: "/home/jbreitbart/.rustup/toolchains/nightly-2020-09-10-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/Cargo.lock" does not exist, unable to build with the standard library, try:
        rustup component add rust-src

 ~/temp/libhermit-rs on  master *
 ᨂ rustup component add rust-src
info: downloading component 'rust-src'
info: installing component 'rust-src'
info: Defaulting to 500.0 MiB unpack ram

 ~/temp/libhermit-rs on  master *
 ᨂ cargo build
  Downloaded hashbrown v0.8.2
  Downloaded 1 crate (75.8 KB) in 0.69s
...

This works only if the current working directory is libhermit-rs. But yes, maybe a good solution.

In addition, I could remove the dependency to nightly compiler in the "user space". I have to check it.

@stlankes
Copy link
Contributor Author

The user space doesn't depend on the nightly compiler. However, I need the latest version of cargo to use the flag -Z or the unstable section in .cargo/config.

@stlankes
Copy link
Contributor Author

I agree and close this PR

@stlankes stlankes closed this Nov 23, 2020
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.

None yet

4 participants