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

feat: use git-version instead of git2 #173

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

mbrobbel
Copy link
Member

@mbrobbel mbrobbel commented Apr 8, 2024

An alternative approach for #172. This replaces the git2 dependency with git-version which uses the git binary directly at compile time. This means that:

For dev builds, needs git binary:

  • There is a submodule checked out: version file is written to the gen folder.
  • There is no submodule checked out: the build fails with a hint to clone the submodule.

For packaged builds (substrait is not included as a git submodule, see Cargo.toml includes), don't need a git binary:

  • The version file is in the gen folder: the build script skips the version generation part
  • The version file is missing: the build fails - the package is broken

Copy link
Member

@westonpace westonpace left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this.

const PROTO_ROOT: &str = "substrait/proto";
const TEXT_ROOT: &str = "substrait/text";
const GEN_ROOT: &str = "gen";
Copy link
Member

Choose a reason for hiding this comment

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

This is a good idea to mark that the code is generated

@westonpace westonpace merged commit 22d6df7 into substrait-io:main Apr 8, 2024
11 checks passed
@westonpace
Copy link
Member

Ah, this doesn't seem to be working quite right. I get an error using the released build (because git_submodule_versions! runs immediately, being a macro, even if version.in exists). I'll investigate.

error: failed to determine .git directory: git rev-parse exited with status 128: fatal: not a git repository (or any of the parent directories): .git
  --> /home/pace/.cargo/registry/src/index.crates.io-6f17d22bba15001f/substrait-0.29.1/build.rs:40:31
   |
40 |           const VERSION: &str = git_version::git_submodule_versions!(
   |  _______________________________^
41 | |             args = ["--tags", "--long", "--dirty=-dirty", "--abbrev=40"],
42 | |             fallback = ""
43 | |         )[0]
   | |_________^
   |
   = note: this error originates in the macro `git_version::git_submodule_versions` (in Nightly builds, run with -Z macro-backtrace for more info)

error: could not compile `substrait` (build script) due to previous error

@mbrobbel
Copy link
Member Author

mbrobbel commented Apr 8, 2024

Ah, this doesn't seem to be working quite right. I get an error using the released build (because git_submodule_versions! runs immediately, being a macro, even if version.in exists). I'll investigate.

error: failed to determine .git directory: git rev-parse exited with status 128: fatal: not a git repository (or any of the parent directories): .git
  --> /home/pace/.cargo/registry/src/index.crates.io-6f17d22bba15001f/substrait-0.29.1/build.rs:40:31
   |
40 |           const VERSION: &str = git_version::git_submodule_versions!(
   |  _______________________________^
41 | |             args = ["--tags", "--long", "--dirty=-dirty", "--abbrev=40"],
42 | |             fallback = ""
43 | |         )[0]
   | |_________^
   |
   = note: this error originates in the macro `git_version::git_submodule_versions` (in Nightly builds, run with -Z macro-backtrace for more info)

error: could not compile `substrait` (build script) due to previous error

Hm, it should use the provided fallback (when the git describe fails), I saw this error while testing, and it went away for me after adding the fallback.

@westonpace
Copy link
Member

Ah, it seems git_versions! respects fallback but not git_submodule_verisons!

@westonpace
Copy link
Member

Ah, nevermind, git_submodule_verisons! does look at fallback but only for some errors.

mbrobbel added a commit that referenced this pull request Apr 8, 2024
@mbrobbel mbrobbel mentioned this pull request Apr 8, 2024
@mbrobbel
Copy link
Member Author

mbrobbel commented Apr 8, 2024

I'll try to create a patch for the upstream crate.

@mbrobbel mbrobbel deleted the git-version branch April 8, 2024 13:11
mbrobbel added a commit that referenced this pull request Apr 8, 2024
Reverts #173, to fix the broken `0.29.1`
release.
@westonpace
Copy link
Member

Thanks. I think the problem is that, with this kind of failure, git_submodule_versions doesn't even know how many tuples to return (because it doesn't know how many submodules there are). Maybe you can add a new functions git_submodule_version which requires a submodule name as input, and which always respects fallback.

@mbrobbel
Copy link
Member Author

mbrobbel commented Apr 8, 2024

Thanks. I think the problem is that, with this kind of failure, git_submodule_versions doesn't even know how many tuples to return (because it doesn't know how many submodules there are). Maybe you can add a new functions git_submodule_version which requires a submodule name as input, and which always respects fallback.

Yes, I was considering the same, but then I figured we can probably be pragmatic here: #175.

westonpace pushed a commit that referenced this pull request Apr 8, 2024
Same idea as #173, except this directly calls `git describe` at build
time (when there is a submodule).
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.

2 participants