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

use Cargo.toml & Cargo.lock for sccache build dependency #3709

Closed
wants to merge 1 commit into from

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Mar 15, 2021

No description provided.

src/main.rs Outdated Show resolved Hide resolved
[build-dependencies]
# sccache's handling of the /fp:fast MSVC compiler option is broken, but this is fixed in the master branch.
# https://github.com/mozilla/sccache/pull/962
sccache = { git = "https://github.com/mozilla/sccache.git", branch = "master" }
Copy link
Member

@Swiftb0y Swiftb0y Mar 15, 2021

Choose a reason for hiding this comment

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

Suggested change
sccache = { git = "https://github.com/mozilla/sccache.git", branch = "master" }
sccache = { git = "https://github.com/mozilla/sccache.git", branch = "master" }
[lib]
path = "non-existent" # required so `cargo update` runs

If you add this, you can remove the main.rs file. works on my machine but try it yourself as well please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool, thanks for this hack

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This lets cargo update work but I think what we actually need on CI is cargo build.

Copy link
Member

Choose a reason for hiding this comment

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

Can't say much about that. I don't understand what this PR is supposed to do anyways. I just figured there must be a better way than leaving a dummy main.rs in the repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of this PR is to use Cargo.lock as the cache key instead of the ugly hack of a git commit hash which won't work well as we add more Rust dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but why do we need to change cargo install --git https://github.com/mozilla/sccache.git --branch master

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for asking stupid questions. The solution seems overcomplicated to me, but thats usually a sign that I don't know enough about the actual topic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current solution works for now. But it won't work when we also have other Rust dependencies to cache.

@Be-ing Be-ing force-pushed the cargo branch 5 times, most recently from b44d2ef to cb6ea07 Compare March 15, 2021 18:09
@Be-ing Be-ing changed the base branch from main to 2.3 March 15, 2021 18:11
@Be-ing Be-ing force-pushed the cargo branch 2 times, most recently from de453f1 to 3e3c324 Compare March 15, 2021 18:28
@github-actions github-actions bot added the build label Mar 15, 2021
@Be-ing
Copy link
Contributor Author

Be-ing commented Mar 15, 2021

It seems that adding a binary as a dependency doesn't install it to $PATH:

-- Could NOT find sccache (missing executable)
-- Support for sccache: OFF

@Holzhaus
Copy link
Member

It seems that adding a binary as a dependency doesn't install it to $PATH:

-- Could NOT find sccache (missing executable)
-- Support for sccache: OFF

Cargo installs to the home dir (on Linux: $HOME/.cargo/bin) and adds that path to $PATH. However, GitHub Actions does not preserve environment variables across job steps. You need to mark explicitly what env cars should be preserved.

This is why we do echo FOO=bar > "$GITHUB_ENV" in the other build steps.
https://docs.github.com/en/actions/reference/environment-variables#about-environment-variables

@Be-ing
Copy link
Contributor Author

Be-ing commented Mar 15, 2021

cargo install --path . only installs the mixxx executable but not sccache. cargo build builds sccache but not the executable...

@Be-ing
Copy link
Contributor Author

Be-ing commented Mar 15, 2021

I think cargo might not support this use case yet but it will be implemented soon... rust-lang/cargo#9096

@Be-ing Be-ing marked this pull request as draft March 15, 2021 20:34
@Holzhaus
Copy link
Member

Can't we define an env var SCCACHE_VERSION, then use the env var in the cache key and use cargo install --git <url> --version %SCCACHE_VERSION% instead of using a lock file and a dummy package?

@uklotzde
Copy link
Contributor

Can't we define an env var SCCACHE_VERSION, then use the env var in the cache key and use cargo install --git <url> --version %SCCACHE_VERSION% instead of using a lock file and a dummy package?

I agree. Referencing the git ref directly instead of using a dummy project might be the way to go in this special case. I didn't think about that.

Maybe obsolete then: Be-ing#55

@Be-ing
Copy link
Contributor Author

Be-ing commented Mar 15, 2021

Can't we define an env var SCCACHE_VERSION, then use the env var in the cache key and use cargo install --git --version %SCCACHE_VERSION% instead of using a lock file and a dummy package?

That would be a bit more complicated than the current implementation for no benefit.

Maybe we could just cache C:\Users\runneradmin\.cargo\bin\sccache instead of all of C:\Users\runneradmin\.cargo. That way we could also have separate caches for other Rust binaries (aoide).

@Be-ing
Copy link
Contributor Author

Be-ing commented Mar 15, 2021

I started a thread on the Rust forum asking for help.

@Holzhaus
Copy link
Member

That would be a bit more complicated than the current implementation for no benefit.

Why? It would be a ~10 line diff, not a >3K line diff.

@Be-ing
Copy link
Contributor Author

Be-ing commented Mar 15, 2021

Your proposal has no advantage over simply using the commit hash as the cache key and referencing the Git branch with cargo install.

@Holzhaus
Copy link
Member

Your proposal has no advantage over simply using the commit hash as the cache key and referencing the Git branch with cargo install.

That is exactly what I proposed?! Just save the commit hash in an env var to avoid duplication of that value for cache key and and cargo install. That way we don't have the risk of updating one but not the other.

@Be-ing
Copy link
Contributor Author

Be-ing commented Mar 15, 2021

There is no duplication of the commit hash currently. cargo install references the branch, not the commit.

@Holzhaus
Copy link
Member

Holzhaus commented Mar 15, 2021

We should pin a specific commit. I don't want our CI builds failing because they introduced a breaking change or broke main upstream.

@Be-ing
Copy link
Contributor Author

Be-ing commented Mar 15, 2021

Okay then you can open another PR to do that.

@Be-ing Be-ing closed this Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants