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

ijar incorrectly handles MANIFESTs with sections #12730

Closed
liucijus opened this issue Dec 18, 2020 · 4 comments
Closed

ijar incorrectly handles MANIFESTs with sections #12730

liucijus opened this issue Dec 18, 2020 · 4 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-Java Issues for Java rules type: bug

Comments

@liucijus
Copy link
Contributor

Description of the problem:

ijar tool incorrectly handles MANIFEST files with sections by removing empty lines and appending main attribute Target-Label after them.
I've faced this issue by trying to use java_common.stamp_jar on external jars.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Repro: https://github.com/liucijus/ijar-stamping-repro

What operating system are you running Bazel on?

Linux (Xubuntu 20.04)

What's the output of bazel info release?

release 3.7.2

@aiuto aiuto added team-Rules-Java Issues for Java rules untriaged labels Jan 6, 2021
@comius comius added P3 We're not considering working on this, but happy to review a PR. (No assignee) type: bug and removed untriaged labels Jan 7, 2021
@ittaiz
Copy link
Member

ittaiz commented Jan 13, 2021

@dslomov @dslomov @comius this is blocking rules_scala's ability to stamp 3rd party jars.
@liucijus sent a really small PR to fix this and we'd really appreciate it if someone could review it

@ittaiz
Copy link
Member

ittaiz commented Jan 29, 2021

@comius may I ask why this is P3?
IMHO this is at least a P2. The reason java rules use stamping is to avoid memory issues of passing long lists of labels.
We (at Wix and rules_scala) have been working for the past 6 months on being able to move to stamping to solve these issues but are facing many issues.
This is one of them.

@comius
Copy link
Contributor

comius commented Jan 29, 2021

@comius may I ask why this is P3?
IMHO this is at least a P2. The reason java rules use stamping is to avoid memory issues of passing long lists of labels.

No, it's P3 - meaning we are accepting community PRs. P2 - somebody in the team would be working on it.

We (at Wix and rules_scala) have been working for the past 6 months on being able to move to stamping to solve these issues but are facing many issues.
This is one of them.

Sorry to hear that. Let me see if I can speed this up a bit. In the future "friendly ping" on the PR would suffice. github has a lot of noise and it's hard to see if there is PR that's been waiting for a long time. Keeping a queue of PRs in my mind just doesn't work everytime.

Ideally we'd review things in a week or two, but there are large discrepancies among PRs, so my guess is I'm not the only one having problems keeping up.

@ittaiz
Copy link
Member

ittaiz commented Jan 29, 2021

No, it's P3 - meaning we are accepting community PRs. P2 - somebody in the team would be working on it.

I see. I was looking through the bazel site for explanation on the priorities meaning and the classification of P3 was something super minor or something along those lines.
I think it's valuable adding your explanation to that doc.

In the future "friendly ping" on the PR would suffice

I did do that twice, just didn't know how to continue...

In any case thank you so much for advancing this!

philwo pushed a commit that referenced this issue Mar 15, 2021
Fixes #12730

Important changes:
 - removed line stripping from the original implementation
 - label/rule attributes are appended after main attributes, then the rest of the `MANIFEST.MF` content is appended - this way sections data is preserved.

Closes #12771.

PiperOrigin-RevId: 354909855
philwo pushed a commit that referenced this issue Mar 15, 2021
Fixes #12730

Important changes:
 - removed line stripping from the original implementation
 - label/rule attributes are appended after main attributes, then the rest of the `MANIFEST.MF` content is appended - this way sections data is preserved.

Closes #12771.

PiperOrigin-RevId: 354909855
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-Java Issues for Java rules type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants