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

Jars should include directories #2136

Closed
tballard opened this issue Nov 22, 2022 · 9 comments · Fixed by #2138 or #2142
Closed

Jars should include directories #2136

tballard opened this issue Nov 22, 2022 · 9 comments · Fixed by #2138 or #2142
Milestone

Comments

@tballard
Copy link

Mill builds jars without entries for directories, but directories are cheap and allow searching for resources and are otherwise harmless and thus should be included. (Evidence suggests that maven includes them by default, supporting the claim of being benign)

@lolgab
Copy link
Member

lolgab commented Nov 22, 2022

Can you provide an example?

@lefou
Copy link
Member

lefou commented Nov 22, 2022

Yeah, I created jars with so many build tools and even coded that part multiple times from scratch for various build tools. It could appear as nitpicking to argue for or against additionally explicit directory entries. I nowadays think, it's probably easier to read and probably also easier to process, when these entries are present. And when you told me, that Spring can't find resources properly when directory entries are missing, you had me. It's probably a poor implementation on their side, but nevertheless, it's inconvenient and avoidable.

Only thing I am very opinionated about is, that we keep the META-INF/MANIFEST.MF entry as first and before the META-INF directory entry. This is more in line with the early specs and also makes it much easier to look into jars, e.g. with zless my.jar, which especially helps if you work with OSGi or Manifests in general a lot.

@tballard
Copy link
Author

I think the directories may be supposed to precede their contents, too

@lefou
Copy link
Member

lefou commented Nov 22, 2022

I think the directories may be supposed to precede their contents, too

Sure. That is also my assumption/understanding. That's why I pointed to the special case with META-INF/.

@lefou
Copy link
Member

lefou commented Nov 23, 2022

@tballard Do you mind to give PR #2138 a try?

@tballard
Copy link
Author

Yeah I will. I'm writing some code to repack the current one, adding the required directories, so I'll be all practiced up.

lefou added a commit that referenced this issue Nov 24, 2022
Move JAR creation code into separate `JarOps` object to create a
`os-lib` like user experience.

Added support for creating directory entries, to fix #2136

Also prepared to use fixed timestamps to assist creating reproducible
jars.

Added use of buffered stream when writing the file.

Pull request: #2138
@lefou lefou added this to the 0.10.10 milestone Nov 24, 2022
@tballard
Copy link
Author

Hmm, I thought I was implementing this, although I like your solution better. However, you will note that if you try to add an entry like META-INF/SOMETHING_ELSE you get a collision:
1 targets failed
reds.jar java.util.zip.ZipException: duplicate entry: META-INF/
java.base/java.util.zip.ZipOutputStream.putNextEntry(ZipOutputStream.java:241)

@lefou
Copy link
Member

lefou commented Nov 24, 2022

Hmm, I thought I was implementing this, although I like your solution better.

Oh, I though you will start it once you are on holiday, but I didn't realized that this means "now".

However, you will note that if you try to add an entry like META-INF/SOMETHING_ELSE you get a collision: 1 targets failed reds.jar java.util.zip.ZipException: duplicate entry: META-INF/ java.base/java.util.zip.ZipOutputStream.putNextEntry(ZipOutputStream.java:241)

Oh, this is an oversight, as the special case META-INF/ needs to be explicitly added to the seen set. Thanks for testing and pointing this out. I'll push a fix.

@tballard
Copy link
Author

Yeah, I had that bug first;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants