Update dependencies

[hannobraun]

No description provided.

[therealprof]

@japaric Shouldn't we rather remote Cargo.lock completely? I think it is rather bad style to check that in for general purpose software.

[pftbest]

@therealprof, Why? This project is a binary, not a library, so it makes sense to have a reproducible build.

http://doc.crates.io/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries

[therealprof]

@pftbest Why exactly do you want a "reproducible build"? This application is neither security relevant nor extremely fragile.

People working with/on svd2rust have constantly changing dependencies and I find it rather annoying having to work around dirty git managed files in the working directory all the time. Judging from this pull request I don't seem to be the only one...

[hannobraun]

I think @therealprof makes a good point. Managing the Cargo.lock in the repository makes most sense for applications that you deploy to a server, or distribute to an end user. Less so for tools that are distributed to developers. On the other hand, this is still an application, so we're not dealing with transitive dependencies.

I'm not sure, but maybe it's better to just remove it.

[pftbest]

If it does inddeed hurt the productivity in this case, then I don't have any objections.

I just don't agree that it's a "bad style". From the end user perspective, when you do cargo install program you get exactly the same version that was tested in CI and is known to work. But if there is no lock file, you never sure what you get.

[hannobraun]

@pftbest Also a good point. This is not what's happening right now though. Cargo.lock is so out of date that Cargo is forced to update the dependency versions on every build (since the versions specified in Cargo.toml are newer than those in Cargo.lock).

Maybe it's possible to fail the Travis build whenever Cargo.lock becomes so out of date again? If we could do that, we could keep it (and keep its benefits) without running the risk of it getting out of date again. Not sure how that would work though. Unless someone else chimes in first, I'll have a look at the Cargo documentation later to figure something out.

[therealprof]

@pftbest I think it is bad style just because usually you don't want a pinned version, you want the latest compatible version which is what semantic versioning is for; of course you could also use that to pin a more or less exact version.

It's one of the main problems of our times that new versions of dependencies are usually not picked up automatically and no one gives a crap to manually update them so there's still a load of software out there shipped with outdated dependencies causing harsh security issues.

In big companies who want to exert precise control over dependencies you have at least one person, potentially a whole team, of release managers who also take care of dependency management. If you don't have that it's a very bad idea.

Whoever put that recommendation in the cargo documentation deserves a decent whack with a clue bat...

[Emilgardis]

I'm agreeing with @pftbest, I think we should keep the Cargo.lock file. See for example rustfmt and rustup.

[kjetilkjeka]

When a piece of software is unstable with unstable dependencies. Doesn't that mean

1. Automatically choosing dependencies other than what is confirmed working have a high chance of breaking something
2. If aiming for stability, there should at least be enough dev effort to update deps.

Because of this, I personally think it makes sense to keep Cargo.lock in VC for now. That means

• Someone has to update Cargo.toml
• They also have to run cargo update and commit Cargo.lock together with Cargo.toml

[therealprof]

@kjetilkjeka That is only true if you depend on git repositories, all other versions can (and should!) be pinned using Cargo.toml. You can really only use Cargo.lock is you have a person tracking all dependencies meticulously and making sure it is properly updated in the git repository when required (e.g. to receive security updates). Actually I'd argue the other way around, especially with young software you want to make sure that you update often and early to get all the big improvements, preventing breakage is what semantic versioning is used for; Cargo.lock has nothing to do with that...

[kjetilkjeka]

Well, the versions won't be "pinned" just by using Cargo.toml. They will rather be specified as a set of versions which is guaranteed compatible through semver properties (just as you're saying, preventing breakage is what semantic versioning is used for).

This means that we will, in theory, have a working piece of software. As long as two minor changes in two libraries together don't trigger a weird bug, we're fine. But by using a Cargo.lock we get a reproducible build, meaning that this bug will exist for everyone or for no one.

I think it's nice that when someone uses cargo install svd2rust the version number uniquely defines the build.

I stand corrected on the part about it being more important for young software. It's more forgiving to not provide a consistent build when the SW isn't stable. And since changes are expected to happen more rapidly it's also more rewarding to let them happen "by them self". (Thanks for the clue bat whack @therealprof )

I also absolutely agree that updating deps can be annoying. But I think that reproducibility of a binary is a good idea, even for security insensitive SW. The ideal case is, of course, to have an unlimited amount of resources to do testing and deps updating. But, as a more realistic alternative, it makes sense to remove the lock file for now. As long as the intention is to reintroduce it before stabilization.

tl;dr I'm sold

[hannobraun]

I did the research I promised. You can execute the following command to ensure Cargo.lock is up to date:

$ cargo generate-lockfile --locked

If the lock file isn't up to date, this will return a non-zero exit code, so it can be used to fail the Travis build.

Following @therealprof's line of reasoning though, this won't be enough. Someone would still need to do the actual update in a timely manner. I've come to agree with this viewpoint, and I'll change this pull request to remove the lock file instead of updating it.

By the way, if someone's looking for a side project: How about a bot that can be set up to periodically update a project's lock file, or even the dependency versions in Cargo.toml? This is usually a simple process. If it fails, the bot could open an issue with the build error, and pause itself until the issue is resolved manually.

[japaric]

(How did this PR turn into a discussion about lockfiles?)

My opinion is that we should keep the lockfile and add the cargo generate-lockfile --locked
command to CI to prevent it from going out of date. But ... today lockfiles are not even checked in
crates.io packages so cargo install never sees the lockfile.

This seems fine to me for now but someone should fix rust-lang/cargo#2263. When that happens we
should add back the lockfile.

@homunkulus r+

[homunkulus]

📌 Commit 89c0c2d has been approved by japaric

[homunkulus]

⌛ Testing commit 89c0c2d with merge 89c0c2d...

[homunkulus]

💔 Test failed - status-appveyor

[therealprof]

@japaric I can tell you why... When working with the sources from git you have to dance around the f'ing lock all the time which is incredibly annoying...

# git status
On branch jmaster
Your branch is up-to-date with 'japaric/master'.

nothing to commit, working tree clean
# cargo build --release
...
    Finished release [optimized] target(s) in 13.32 secs
# git status
On branch jmaster
Your branch is up-to-date with 'japaric/master'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   Cargo.lock

no changes added to commit (use "git add" and/or "git commit -a")

[homunkulus]

⌛ Testing commit 89c0c2d with merge 059fe1a...

[homunkulus]

💔 Test failed - status-appveyor

[japaric]

Appveyor cache problem should be fixed in #152.

@homunkulus retry

[homunkulus]

⌛ Testing commit 89c0c2d with merge 5cc57ba...

[homunkulus]

☀️ Test successful - status-appveyor, status-travis
Approved by: japaric
Pushing 5cc57ba to master...