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.

Sign up for GitHub

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

make crates.io version not depend on the git tool #259

Merged
merged 4 commits into from
Nov 16, 2020
Merged

make crates.io version not depend on the git tool #259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e5cc0de
make crates.io version not depend on the `git` tool
japaric Nov 16, 2020
1783d4a
tweak panic message for decoder / probe-run
japaric Nov 16, 2020
94b90f8
remove "debug" statement
japaric Nov 16, 2020
aaad541
tweak logic
japaric Nov 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,29 @@
use semver::Version;
use std::{env, error::Error, fs, path::PathBuf, process::Command};
use std::{env, error::Error, fs, path::Path, path::PathBuf, process::Command};

fn main() -> Result<(), Box<dyn Error>> {
// Put the linker script somewhere the linker can find it
let out = &PathBuf::from(env::var("OUT_DIR")?);
let mut linker_script = fs::read_to_string("defmt.x.in")?;
let output = Command::new("git").args(&["rev-parse", "HEAD"]).output()?;
let version = if output.status.success() {
String::from_utf8(output.stdout).unwrap()
let hash = Command::new("git")
.args(&["rev-parse", "HEAD"])
.output()
Copy link
Contributor

Choose a reason for hiding this comment

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

The output() call itself will fail if the program cannot be found, so I don't think the fix can use .map

Copy link
Member Author

Choose a reason for hiding this comment

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

the original was not checking the .git path if git was not installed but the new version should

.ok()
.and_then(|output| {
if output.status.success() {
String::from_utf8(output.stdout).ok()
} else {
None
}
});
let version = if let Some(hash) = hash {
hash
} else {
assert!(
!Path::new(".git").exists(),
"you need to install the `git` command line tool to use the git version of `defmt`"
);

// no git info -> assume crates.io
let semver = Version::parse(&std::env::var("CARGO_PKG_VERSION")?)?;
if semver.major == 0 {
Expand Down
26 changes: 22 additions & 4 deletions decoder/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,31 @@
use std::{env, error::Error, fs, path::PathBuf, process::Command};
use std::{
env,
error::Error,
fs,
path::{Path, PathBuf},
process::Command,
};

use semver::Version;

fn main() -> Result<(), Box<dyn Error>> {
let out = &PathBuf::from(env::var("OUT_DIR")?);
let output = Command::new("git").args(&["rev-parse", "HEAD"]).output()?;
let version = if output.status.success() {
String::from_utf8(output.stdout).unwrap()
let hash = Command::new("git")
.args(&["rev-parse", "HEAD"])
.output()
.ok()
.and_then(|output| {
if output.status.success() {
String::from_utf8(output.stdout).ok()
} else {
None
}
});
let version = if let Some(hash) = hash {
hash
} else {
assert!(!Path::new(".git").exists(), "you need to install the `git` command line tool to install the git version of `probe-run`");
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you test that this is the right path? I'm not sure in which working directory build scripts are executed. If it's the Cargo workspace root this seems correct, but it seems like it might be the package root, which means that this would be wrong for the decoder.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, there's an old commit that removes a panic!("{:?}, env::current_dir()) statement from the build script. I used to inspect the directory from which the build script was invoked. It was the package root and not the workspace root.

I guess I could also use CARGO_MANIFEST_DIR (was that the path to the directory that contains the Cargo.toml file?) instead of relying on current_dir. Perhaps that'd be more reliable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Either way seems fine to me


// no git info -> assume crates.io
let semver = Version::parse(&std::env::var("CARGO_PKG_VERSION")?)?;
if semver.major == 0 {
Expand Down