rmannibucau commented on PR #220:

   @hboutemy you merged but I didn't see any test showing that it helps, can 
you try to work on it - not asking for an IT, no worries.
   For example a trivial test has the same bug than before the fix so wonder if 
we should revert the code change and work on a better fix or just let it like 
       public void testDST() throws IOException, MojoExecutionException {
           final ZoneOffset entryOffset = ZoneOffset.ofHours(2);
           final Instant entryInstant = LocalDateTime.of(2024, Month.APRIL, 14, 
20, 0).toInstant(entryOffset);
           final Instant entryExtraInstant = LocalDateTime.of(2024, 
Month.APRIL, 14, 20, 1).toInstant(entryOffset);
           final File input = tmp.newFile("in.jar");
           try (final JarOutputStream jar = new 
JarOutputStream(Files.newOutputStream(input.toPath()))) {
               final X5455_ExtendedTimestamp x5455 = new 
               final JarEntry entry = new JarEntry("test.txt");
ZipExtraField[]{ x5455 }));
           final File output = new File(tmp.getRoot(), "output.jar");
           final ShadeRequest request = new ShadeRequest();
           final DefaultShader shader = new DefaultShader();
           final Instant outInstant;
           try (final JarFile out = new JarFile(output)) {
               outInstant = 
           assertEquals(entryExtraInstant, outInstant);
   will fail with:
   Expected :2024-04-14T18:01:00Z
   Actual   :2024-04-14T16:00:00Z
   So hour is wrong (offset management corrupted the value since it is not from 
a jar we manage but a jar we consume?) and time is likely wrong too (wrong time 
is read).
   Main issue for me is that this fix seems to mix dates (entry time and extra 
field time - times in practise) so not really sure it is reliable.
   However it highlights the fact we don't propagate extra fields which can be 
another trivial issue to handle (just mentionning to enforce even more the date 
issue, not asking for a fix for this ticket).
   In terms of fix, what do you intend to do: use the entry time so ignore the 
x5455 field? use the x5455 create time? use the x5455 modify time? ...
   What is wrong using the `Date` timestamp? (`((X5455_ExtendedTimestamp) 
field).getCreateJavaTime()` for ex), how does it affect negatively reproducible 
builds since it is a timestamp related to the UTC "0" (1970)?
   About building in UTC I think I would just enable to set default `ZoneId` 
since `java.util.zip.ZipUtils#javaEpochToLocalDateTime` uses 
`ZoneId.systemDefault()` so it is mainly a matter of calling 
`TimeZone.setDefault` before the build and after the build.
   I was more thinking to do it with java system properties but ultimately we 
could allow to do it in the mojo too and ultimately enable maven to do it per 
mojo (needs a javaagent to modify the jvm default impl to make it contextual 
but this is not a big deal, @gnodet already did it for mvnd and technically 
maven would be the same kind of trick - even easier actually there.
   So long story short: there is a date selection/handling to fix, the timezone 
issue is likely not one and we still have a workaround to not need it (doc 
   Side note: this extra field is only valid until 2037 so it can be sane to 
just ignore it and use the entry time as this since we don't know the original 
offset and we can't invent it until we have the extra field which is known as 
UTC relative.

This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org

For queries about this service, please contact Infrastructure at:

Reply via email to