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: delete files containing git properties #360

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

algomaster99
Copy link
Contributor

Reference: git-commit-id/git-commit-id-maven-plugin#825

The files git.json and git. properties are generated by https://github.com/git-commit-id/git-commit-id-maven-plugin. We delete these files in the jar file to stabilize. We assume default configuration that the files are either called git.json or git.properties.

@algomaster99
Copy link
Contributor Author

@msuozzo ping for review

@msuozzo msuozzo self-requested a review February 21, 2025 23:49
// [here](https://github.com/git-commit-id/git-commit-id-maven-plugin/blob/95d616fc7e16018deff3f17e2d03a4b217e55294/src/main/java/pl/project13/maven/git/GitCommitIdMojo.java#L454).
// By default, these file are created in ${project.build.outputDirectory} and are hence at the root of the jar.
// We assume that the file name is 'git' as this is the default value for the plugin.
// However, the plugin allows customizing the file name.
Copy link
Member

Choose a reason for hiding this comment

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

good detail. have you seen anyone customize this in the wild?

Copy link
Contributor Author

@algomaster99 algomaster99 Feb 22, 2025

Choose a reason for hiding this comment

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

Yes, I saw it here. They customize the file path but not the file name. This project is part of Reproducible Central dataset so it was included in my analysis.

Copy link
Member

Choose a reason for hiding this comment

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

oh wow so they actually allow configuring the path and file to be anything.

one option would be to go snooping for the pom.xml file, parse it, look for this config block, and determine where we'd expect to find the metadata (that is, assuming the configuration can't be injected with e.g. a packaging flag or env var that doesn't end up in the pom). it's overkill but i'm concerned that this being mutable makes this a trickier stabilizer to get to be accurate and/or precise.

thoughts? [not requesting anything be implemented yet]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could try doing so. Our parser should look for plugins whose artifact ID is git-commit-id-maven-plugin in all **/*/pom.xml. This could be declared under plugins or under profiles.plugins. profiles activate a subset of plugins based on the flags provided to the command. Once we do that, we can check its configuration. There is also a go library to parse POMs but does not seem very active. But a simple XML parser could also do the job.

@algomaster99
Copy link
Contributor Author

It seems that there is a convention to group import declarations. However, gofmt did not take care of 1946914 (#360) as I had to do it manually.

@algomaster99
Copy link
Contributor Author

By the way, we will also have to declare the new stabilizers we write under AllZipStabilizers, right?

@algomaster99 algomaster99 requested a review from msuozzo February 22, 2025 08:10
@msuozzo
Copy link
Member

msuozzo commented Feb 24, 2025

It seems that there is a convention to group import declarations. However, gofmt did not take care of 1946914 (#360) as I had to do it manually.

weird. wouldn't worry too much about it as long as gofmt isn't actively complaining

By the way, we will also have to declare the new stabilizers we write under AllZipStabilizers, right?

potentially? alternatively, we might add a separate JAR-specific top-level constant that we also expose in the set of all stabilizers in the CLI. Since it's not specific to the zip format but rather the JAR format, it might be worth keeping them separate.

@algomaster99
Copy link
Contributor Author

wouldn't worry too much about it as long as gofmt isn't actively complaining

👍

potentially? alternatively, we might add a separate JAR-specific top-level constant

Cool. We can take this up in a separate 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