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

[tech] Add a precise git_version to transit-model #754

Merged
merged 5 commits into from
Mar 19, 2021

Conversation

antoine-de
Copy link
Contributor

@antoine-de antoine-de commented Mar 4, 2021

usefull for workflows depending directly of the git repository

seems to handle well the incremental compilation:

❯ cargo build --all
   Compiling transit_model v0.34.0 (/home/antoine/dev/kisio/transit_model)
   Compiling ntfs2gtfs v1.1.0 (/home/antoine/dev/kisio/transit_model/ntfs2gtfs)
   Compiling transit_model_builder v0.3.0 (/home/antoine/dev/kisio/transit_model/model-builder)
   Compiling restrict-validity-period v1.0.0 (/home/antoine/dev/kisio/transit_model/restrict-validity-period)
   Compiling gtfs2ntfs v1.1.0 (/home/antoine/dev/kisio/transit_model/gtfs2ntfs)
   Compiling gtfs2netexfr v1.1.0 (/home/antoine/dev/kisio/transit_model/gtfs2netexfr)
   Compiling ntfs2ntfs v1.0.0 (/home/antoine/dev/kisio/transit_model/ntfs2ntfs)
   Compiling ntfs2netexfr v1.0.0 (/home/antoine/dev/kisio/transit_model/ntfs2netexfr)
    Finished dev [unoptimized + debuginfo] target(s) in 15.30s

❯ cargo build --all
    Finished dev [unoptimized + debuginfo] target(s) in 0.05s
❯ cargo build --all && ./target/debug/gtfs2ntfs --version
    Finished dev [unoptimized + debuginfo] target(s) in 0.05s
gtfs2ntfs v0.34.0-3-g3f518f2-modified

⚠️ do_not_merge is there because we want to use that PR to test our CI (internal webhooks), let's wait until 2021/03/25 max on that 🙏

usefull for workflows depending directly of the git repository
Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

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

Now that I see this PR, I realize that it might just be doing something wrong. gtfs2ntfs --version should output 1.1.0, not the version of transit_model. And ntfs2netexfr should output 1.0.0, not the version of transit_model.

My point is that the output should probably be something like the following (output the version of the binary and of one of the main dependency).

gtfs2ntfs --version
v1.1.0 (transit_model: v0.34.0-3-g3f518f2-modified)

@antoine-de
Copy link
Contributor Author

grumpf, thus this is quite more verbose 😜

but now:

❯ cargo build --all && ./target/debug/ntfs2netexfr --version
ntfs2netexfr 1.0.0 (transit_model = v0.34.0-3-g0847a0b-modified)

I'm off next week, you can continue on this PR if you want (or we can wait one week)

the ci seems not to have git at all steps, add a fallback
@antoine-de
Copy link
Contributor Author

I don't know how to factorize it between binaries, if I put the function in the lib, I get the lib CARGO_PKG_VERSION

Copy link
Contributor

@pbougue pbougue left a comment

Choose a reason for hiding this comment

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

👍 elegant way to track versions (not sure about the final use of it for docker-packaged binaries though)

src/version_utils.rs Show resolved Hide resolved
src/version_utils.rs Show resolved Hide resolved
ntfs2ntfs/src/main.rs Show resolved Hide resolved
Comment on lines +23 to +25
pub fn binary_full_version(binary_version: &str) -> String {
format!("{} (transit_model = {})", binary_version, GIT_VERSION)
}
Copy link
Contributor

@pbougue pbougue Mar 17, 2021

Choose a reason for hiding this comment

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

To be able to track more than one deps (and reuse it in binaries using libs that depends on tm), should we already do something like:

Suggested change
pub fn binary_full_version(binary_version: &str) -> String {
format!("{} (transit_model = {})", binary_version, GIT_VERSION)
}
pub fn binary_full_version(binary_version: &str, other_deps_versions: &str) -> String {
format!("{} ({} transit_model = {})", binary_version, other_deps_versions, GIT_VERSION)
}

The deps-separating , could be handled via an option on other_deps_versions, or just asked-for into this other_deps_versions (like this suggestion).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hum don't you think the other dependency will have it's own way of tracking the version? i don't really think it's the right place to put it but maybe we can talk about it.

Copy link
Contributor

@pbougue pbougue Mar 18, 2021

Choose a reason for hiding this comment

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

OK, so the plan in binaries-that-depends-on-lib-depending-on-TM is to pass GIT_VERSION of the lib-depending-on-TM into binary_version (and forget about CARGO_PKG_VERSION of binaries).

@pbougue pbougue merged commit 1babf91 into hove-io:master Mar 19, 2021
@ArnaudOggy ArnaudOggy changed the title add a precise git_version to transit-model [tech] Add a precise git_version to transit-model Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants