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

Handle bundle folders in VirtualArtifactRepository.transferArtifact #1008

Merged
merged 1 commit into from
Dec 15, 2023

Conversation

merks
Copy link
Contributor

@merks merks commented Dec 14, 2023

The implementation currently assumes all bundles are jars, but bundles from the shared bundle pool may be unpacked as folders.

#962

} else {
File zipFile = null;
try {
zipFile = File.createTempFile(bundleLocation.getName(), ".jar", null); //$NON-NLS-1$
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use a ZipOutputStream instead of creating a physical file to open a stream fro it and copy it to the output stream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's better. Done.

Copy link
Member

Choose a reason for hiding this comment

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

A few lines downwards we have used a JarOutputStream.
Is there a reason for the difference? I wonder if just a ZipOutputStream would be sufficient down there too?
Looking at JarOutputStream the main addition to ZipOutputStream seems to be that it adds a META-INF/MANIFEST.MF entry and has a special magic-number for zips.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would probably better for them to be consistent.,

Copy link
Contributor

Choose a reason for hiding this comment

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

JarOutputStream looks like a good idea, even though I think this is just jared to be extracted right away anyways but consistency is probably key here :-)

Copy link

github-actions bot commented Dec 14, 2023

Test Results

     270 files  +       6       270 suites  +6   52m 4s ⏱️ + 9m 30s
  3 329 tests ±       0    3 299 ✔️ +       2  30 💤 ±  0  0  - 2 
10 284 runs  +2 701  10 194 ✔️ +2 679  90 💤 +24  0  - 2 

Results for commit 77bb7b8. ± Comparison against base commit 75fdc0d.

♻️ This comment has been updated with latest results.

The implementation currently assumes all bundles are jars, but bundles
from the shared bundle pool may be unpacked as folders.

eclipse-pde#962
@merks merks force-pushed the pr-transfer-bundle-folder branch from 77df22c to 77bb7b8 Compare December 14, 2023 14:52
Copy link
Contributor

@laeubi laeubi left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

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

Thank you Ed for this fix.
I just have a few minor remarks. Please see them below.

Comment on lines +133 to +135
try (InputStream is = new FileInputStream(bundleLocation)) {
is.transferTo(destination);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
try (InputStream is = new FileInputStream(bundleLocation)) {
is.transferTo(destination);
}
Files.copy(bundleLocation.toPath(), destination);

@laeubi laeubi merged commit 02ddc95 into eclipse-pde:master Dec 15, 2023
14 of 17 checks passed
@laeubi
Copy link
Contributor

laeubi commented Dec 15, 2023

I'm merging this now and create a follow-up PR shortly.

@merks merks deleted the pr-transfer-bundle-folder branch December 15, 2023 07:21
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.

3 participants