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

Add Support for Submodule Versioning #23

Merged
merged 22 commits into from
Dec 11, 2023
Merged

Add Support for Submodule Versioning #23

merged 22 commits into from
Dec 11, 2023

Conversation

baxterjo
Copy link
Contributor

@baxterjo baxterjo commented Nov 5, 2023

Added new git_version_modules macro that uses a combination of git submodule foreach and git describe to format a block of text that accomplishes what git_version does for the root repository.

Use

Given the file structure of this PR:

.
├── COPYING
├── Cargo.lock
├── Cargo.toml
├── README.md
├── git-version-macro
│   ├── COPYING -> ../COPYING
│   ├── Cargo.toml
│   └── src
│       ├── describe_submodules.rs
│       ├── lib.rs
│       └── utils.rs
├── rustfmt.toml
├── src
│   └── lib.rs
├── test-child-repo
│   ├── README.md
│   └── test-grandchild-repo
│       └── README.md
├── test_outer_directory
│   └── test-child-repo
│       ├── README.md
│       └── test-grandchild-repo
│           └── README.md
└── tests
    └── version.rs
use git_version::git_version_submodule;

let module_versions = git_version_modules!(
		prefix = "pre-",
		suffix = "-suff",
		describe_args = ["--always", "--dirty=-modified", "--tags"]
	);

Example output:

[
	["test-child-repo", "pre-da418ba-suff"],
	["test-child-repo/test-grandchild-repo", "pre-12551c3-suff"],
	["test_outer_directory/test-child-repo ", "pre-da418ba-suff"],
	["test_outer_directory/test-child-repo/test-grandchild-repo", "pre-12551c3-suff"],
]

@de-vri-es
Copy link
Collaborator

de-vri-es commented Nov 15, 2023

Thanks! It could certainly be interesting to support information for submodules too.

But I think the macro should expand to an array of submodule entries, giving the path and version information of each. Now it just expands to one string literal?

@baxterjo
Copy link
Contributor Author

@de-vri-es This is ready for re-review.

@de-vri-es
Copy link
Collaborator

Output looks good. The only thing I'm contemplating is if we should use a different type for the submodule entries. A tuple seems more logical than a [&str; 2], but maybe we should even use a struct with named fields? Something like SubmoduleVersion { path: ..., version: ... } maybe. What do you think?

(Didn't look at the implementation yet.)

for line in newline_split {
let line = line.to_string();
let line_split: Vec<&str> = line.split(':').collect();
assert!(
Copy link
Collaborator

@de-vri-es de-vri-es Nov 21, 2023

Choose a reason for hiding this comment

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

The proc macro should never panic. This should be a compile error instead. But I also think the check should probably be removed. If people use colons in their tags, it should still just work.

But maybe we shouldn't use git submodule foreach, but just get a list of submodules and do the foreach ourselves. Then we avoid the need to parse output at all (what if the submodule path also has a colon?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, and removed foreach args. Also renamed describe_args back to args to be more inline with the git_version macro.

@baxterjo
Copy link
Contributor Author

baxterjo commented Nov 23, 2023

Output looks good. The only thing I'm contemplating is if we should use a different type for the submodule entries. A tuple seems more logical than a [&str; 2], but maybe we should even use a struct with named fields? Something like SubmoduleVersion { path: ..., version: ... } maybe. What do you think?

(Didn't look at the implementation yet.)

@de-vri-es I think I want to avoid using custom structs because I personally like using the git_version macro to define a const at the top of the file. And since consts cannot have their type inferred, you have to declare the type in the array and then that creates a circular dependency with the macro crate.

I think an output of [(&str,&str), N] is good. Although it would force users to change the number N whenever a submodule is added, I wouldn't expect users to be changing that number often over the course of their projects.

@de-vri-es
Copy link
Collaborator

de-vri-es commented Nov 23, 2023

@de-vri-es I think I want to avoid using custom structs because I personally like using the git_version macro to define a const at the top of the file. And since consts cannot have their type inferred, you have to declare the type in the array and then that creates a circular dependency with the macro crate.

It wouldn't be a technical problem, but I agree, it's nice to limit the number of types/functions you need. So fair, lets not go for a custom struct.

I was also thinking maybe the submodule path should still be &Path instead of &str. But I think in most cases &str is actually easier to work with. So lets also stick with &str for the path.

I think an output of [(&str,&str), N] is good. Although it would force users to change the number N whenever a submodule is added, I wouldn't expect users to be changing that number often over the course of their projects.

Yeah, an array is good. If people want, they can easily turn it into a slice by taking a reference:

const SUBMODULE_VERSIONS: &[(&str, &str)] = &git_version_modules!();

By expanding to an array, we do give people the option to use N at compile time.

One more bikeshed point: I think git_submodule_versions!() is probably a better name for the macro.

.gitmodules Outdated
@@ -0,0 +1,6 @@
[submodule "test-child-repo"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be nice to set up a custom testing repo for this instead of adding submodules. But I will take care of this part, since I also think they should be in the same organization :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed submodules and added more effective test. Also fixed some issues that popped up when this macro is ran in a project with no submodules.

@baxterjo
Copy link
Contributor Author

baxterjo commented Nov 28, 2023

One more bikeshed point: I think git_submodule_versions!() is probably a better name for the macro.

@de-vri-es Renamed the macro, also updated docs for usage.

@baxterjo
Copy link
Contributor Author

baxterjo commented Dec 6, 2023

@de-vri-es ping

git-version-macro/src/describe_submodules.rs Outdated Show resolved Hide resolved
git-version-macro/src/describe_submodules.rs Outdated Show resolved Hide resolved
Comment on lines +126 to +134
let mut args: Vec<String> = "submodule foreach --quiet --recursive"
.to_string()
.split(' ')
.map(|x| x.to_string())
.collect();

args.push("echo $displaypath".to_string());

let result = run_git("git submodule", Command::new("git").args(args))?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let mut args: Vec<String> = "submodule foreach --quiet --recursive"
.to_string()
.split(' ')
.map(|x| x.to_string())
.collect();
args.push("echo $displaypath".to_string());
let result = run_git("git submodule", Command::new("git").args(args))?;
let result = run_git(
"git submodule",
Command::new("git")
.arg("submodule")
.arg("foreach")
.arg("--quiet")
.arg("--recursive")
.arg("echo $displaypath"),
)?;

Copy link
Collaborator

@de-vri-es de-vri-es Dec 7, 2023

Choose a reason for hiding this comment

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

Does echo $displaypath work on windows though? Maybe we should parse the .gitmodules file instead relying on git submodule foreach.

Copy link
Contributor Author

@baxterjo baxterjo Dec 8, 2023

Choose a reason for hiding this comment

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

Lemme check real quick.

Edit: Yep! It works. The shell that executes the command must be implemented by git. Tested on Powershell and Command Prompt

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok :)


}))
}
Err(_) if args.fallback.is_some() => Ok(quote!([("fallback", args.fallback)])),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure this makes sense here. For git_version!(), the fallback means: if "git describe fails, give this as version instead".

I think the fallback should have the same meaning for submodules: each submodule entry should apply the fallback independently if git describe fails.

At the very least, it will mean that the submodule paths are always real paths. With the current interpretation of the fallback argument, you can't distinguish between the fallback being used or a submodule at path "fallback".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it felt clunky. I will implement this tomorrow.

Copy link
Contributor Author

@baxterjo baxterjo Dec 8, 2023

Choose a reason for hiding this comment

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

@de-vri-es Fixed up to work as you described. Will conditionally add a fallback on a per-submodule basis if a fallback is provided, and throw a compiler error otherwise, just like git_version!().

@de-vri-es de-vri-es self-assigned this Dec 9, 2023
@de-vri-es de-vri-es merged commit 14f3942 into fusion-engineering:master Dec 11, 2023
2 checks passed
@de-vri-es
Copy link
Collaborator

Released as v0.3.9 🚀

Thanks for the PR!

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