Skip to content
This repository has been archived by the owner on Feb 28, 2023. It is now read-only.

Adds SUBSTRATE_ env variables to hash #83

Closed
wants to merge 1 commit into from

Conversation

crystalin
Copy link

This allow to be more consistent when building substrate node, as the substrate uses the environment variable SUBSTRATE_CLI_IMPL_VERSION to propagate the version:

	println!("cargo:rustc-env=SUBSTRATE_CLI_IMPL_VERSION={}", get_version(&commit))

This environment variable was ignored by SCCache preventing the binary to display the right version

@drahnr
Copy link
Contributor

drahnr commented Jul 3, 2021

If a env variable is used with rustc-env from a build.rs, it will be included by a different logic the dependency file.

Within that file all file dependencies as well as env variables will be included. If you enable trace output you can see the dependency file for a certain rust source file enabling trace output: https://github.com/paritytech/sccache/blob/master/src/compiler/rust.rs#L227 (see the documentation how to enable this).

fn parse_dep_info would need to be able to parse # env-dep:VAR=VALUE and there is a need to extract this from the depfile as well rather than just file paths. The rust compiler repo has a few more infos on the behavior https://github.com/rust-lang/rust/tree/master/src/test/run-make/env-dep-info

So for now we can merge this if it makes ends meet, but rather not and impl the depfile parsing logic. I am happy to mentor if needed :)

@crystalin
Copy link
Author

@drahnr I agree, I made this as a quick solution but using the dep info is better. I'm not too familiar with the project (not too much with rust neither), so I'll see if someone from our team can look at that

@drahnr drahnr mentioned this pull request Jul 6, 2021
@drahnr
Copy link
Contributor

drahnr commented Jul 6, 2021

@crystalin I took a quick stab at it, see #84

@drahnr drahnr closed this Jul 7, 2021
drahnr added a commit that referenced this pull request Nov 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants