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

Stamp scalac jars #1178

Merged
merged 4 commits into from
Jan 15, 2021
Merged

Stamp scalac jars #1178

merged 4 commits into from
Jan 15, 2021

Conversation

liucijus
Copy link
Collaborator

ScalacWorker stamps jars with target label

@@ -265,8 +266,18 @@ public static void buildJar(String[] args) throws IOException {
manifestFile = args[1];
idx = 2;
}

String targetLabel = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably usage message needs to be updated. Current impl requires flags in specific order ie -m ... -t ... maybe need to be more flexible here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to do a minimal change here - it's a copy/paste from bazel: https://github.com/bazelbuild/bazel/blob/master/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/jarhelper/JarCreator.java

I think better solution would be to research and find a way to depend on the original code and extend/override its functionality. But that's probably too big for this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added info to usage string

Copy link
Member

@ittaiz ittaiz left a comment

Choose a reason for hiding this comment

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

one question- is the name “Target-Label” also what java uses?

@liucijus
Copy link
Collaborator Author

@ittaiz
Copy link
Member

ittaiz commented Jan 15, 2021

👍 looks good. Resolve the conflict and merge

@liucijus liucijus merged commit cf4e507 into bazelbuild:master Jan 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants