Skip to content

Pin dependencies exactly - #14

Closed
ojeda wants to merge 1 commit into
rustfrom
pin-deps
Closed

Pin dependencies exactly#14
ojeda wants to merge 1 commit into
rustfrom
pin-deps

Conversation

@ojeda

@ojeda ojeda commented Sep 20, 2020

Copy link
Copy Markdown
Member

See #12

This ensures reproducible builds and no unintended updates even if a developer runs cargo on their own without one of the locking flags. We will explicitly manage them.

It is likely that we will change how we manage the dependencies, but for the moment, it is good to have the semantics we will anyway want.

@ojeda
Pin dependencies exactly
5510d3a 
This ensures reproducible builds and no unintended updates
even if a developer runs `cargo` on their own without one
of the locking flags. We will explicitly manage them.

Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
@alex

alex commented Sep 20, 2020

Copy link
Copy Markdown
Member

We have a lockfile for the workspace, what does this pin?

@ojeda

ojeda commented Sep 20, 2020

Copy link
Copy Markdown
Member Author

Cargo.lock does not actually lock anything on its own unless you tell Cargo to not modify it, no? This is intended to prevent that. It also makes more sense to actually pin the versions themselves since that is what we want. Also, since we run with --locked, if one of the dependencies updates, it would break the build, no?

@kloenk

kloenk commented Oct 10, 2020

Copy link
Copy Markdown
Member

What do you mean with break?
--locked will prevent cargo from updating the lockfile. if you want to update you could just run cargo update if this PR does not get merged. Cargo should make sure that everything still builds, as there is only one big lockfile.

@ojeda

ojeda commented Oct 10, 2020

Copy link
Copy Markdown
Member Author

So the man page says:

If the lock file ... needs to be updated, Cargo will exit with an error.

Therefore, if a dependency gets updated and we haven't pinned them, Cargo will want to update it, which means an error, no?

Then there is --frozen. From the man page, it looks like it avoids using the network to see if new versions are available, but from the wording I'd assume it can still check the local index and complain. Then there is --offline, which it is not clear to me from the man page what is the exact interaction with the other flags.

What we want is that code never changes, for any reason whatsoever, unless someone explicitly changes it. Regardless of Cargo's flags, it sounds like explicitly writing what we want is a good idea.

Related: the different behavior of cargo install vs. cargo build w.r.t. the lockfile.

@alex

alex commented Oct 10, 2020 via email

Copy link
Copy Markdown
Member

@ojeda

ojeda commented Oct 11, 2020

Copy link
Copy Markdown
Member Author

The lock file would only need to be updated if Cargo.toml changed, a new release doesn't mean the lock file needs to be updated, just that it could be updated.

Ah, so by "out of date" the manpage means w.r.t. to the Cargo.toml, not w.r.t. a new released version. Got it, that was the bit I was missing, thanks.

@ojeda

ojeda commented Oct 11, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Alright, then yeah, my main concern is gone. Since we don't use cargo install (which ignores the lockfile in some cases), the only difference pinning would make (for us) would be being more explicit about the version in the Cargo.toml (and therefore requiring changes to it and not just Cargo.lock, effectively duplicating some of the information). Is that right?

In that case, well, I don't mind one way or the other. I always prefer to be explicit on dependencies management, but I don't like repeated information either; and it is not how usual Rust projects handle dependencies with Cargo; so we can close this.

@kloenk

kloenk commented Oct 11, 2020

Copy link
Copy Markdown
Member

I would vote in favor of not using cargo, as it makes it quiet complicated to use for #23.
@ojeda or so, would anyone like to create an IRC or matrix room? As I'm not that good with kbuild, maybe someone wants to help me there?

@kloenk

kloenk commented Oct 11, 2020

Copy link
Copy Markdown
Member

I'm trying call bindgen as an executable and not from the build.rs. As this would us only let with bitflags as dependency, which does not have any dependencies.
But currently, I'm not sure where in the makefile to call bindgen

@alex

alex commented Oct 11, 2020 via email

Copy link
Copy Markdown
Member

@kloenk

kloenk commented Oct 11, 2020

Copy link
Copy Markdown
Member

How about shelling out to it in build.rs?

As I would like to ditch cargo, the is no build.rs left

@alex

alex commented Oct 11, 2020 via email

Copy link
Copy Markdown
Member

@kloenk

kloenk commented Oct 11, 2020

Copy link
Copy Markdown
Member

We only have one dependency. This is quite easy to implement in plain rustc. Also without cargo, I would just save bitflags in tree, that avoids any network traffic.

@alex

alex commented Oct 11, 2020 via email

Copy link
Copy Markdown
Member

@kloenk

kloenk commented Oct 11, 2020

Copy link
Copy Markdown
Member

Ok, I understand, but don't like that argument.

What about vendoring bitflags in tree and only using bingen as executable, to make building easier?

@alex

alex commented Oct 11, 2020 via email

Copy link
Copy Markdown
Member

@kloenk

kloenk commented Oct 11, 2020

Copy link
Copy Markdown
Member

If I see it correctly, there are 3 people actively working on this (@alex @ojeda @kloenk ). Do we maybe want to do something like jitsi, and resolve this question. I feel GitHub thread is kinda complicated for this.

@ojeda

ojeda commented Oct 11, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

I would vote in favor of not using cargo, as it makes it quiet complicated to use for #23.

The Cargo-or-not discussion will be forever a hot topic until we get this merged one way or the other. @nickdesaulniers and I started testing the waters without it, and then in a couple of calls we had before Plumbers we (see #1 for the list of people) discussed testing Cargo. See also #4 for some points I wrote about Cargo when I did the initial integration.

At the moment, the main reason/advantage for it is that it contains the knowledge of how to build core and alloc. Whether we use Cargo or not in the end is not that important (in my opinion) since we are using it as a behind-the-scenes build script, i.e. kernel developers won't interact with it.

@ojeda or so, would anyone like to create an IRC or matrix room? As I'm not that good with kbuild, maybe someone wants to help me there? [...] If I see it correctly, there are 3 people actively working on this (@alex @ojeda @kloenk ). Do we maybe want to do something like jitsi, and resolve this question. I feel GitHub thread is kinda complicated for this.

For communication purposes, we will have a mailing list as is usual for kernel stuff (request still pending). For real-time discussion I wanted to have an informal call from time to time like the ones we had for Plumbers. I think it is about a good time to have one since there is a fair amount of bits to discuss after this initial phase. I can send a poll for e.g. next weekend.

@ojeda

ojeda commented Nov 28, 2020

Copy link
Copy Markdown
Member Author

Let's close this one. The bindgen discussion follows in #33, the rest perhaps in the ML! :-)

@ojeda ojeda closed this Nov 28, 2020
@ojeda
ojeda deleted the pin-deps branch November 28, 2020 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ojeda @alex @kloenk