[MSHADE-471] deal with DST

[hboutemy]

like codehaus-plexus/plexus-archiver@b9ea3bf

[rmannibucau]

Ultimately it could need some unit test (or it but it is overkill there and hard to do right) + what about documenting how to not need it (local=utc for the build)?

[hboutemy]

I'd love, if you find a way to do it

[rmannibucau]

@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 that:

    @Test
    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 X5455_ExtendedTimestamp();
            x5455.setCreateFileTime(FileTime.from(entryExtraInstant));

            final JarEntry entry = new JarEntry("test.txt");
            entry.setTime(entryInstant.toEpochMilli());
            entry.setExtra(ExtraFieldUtils.mergeLocalFileDataData(new ZipExtraField[]{ x5455 }));

            jar.putNextEntry(entry);
            jar.closeEntry();
        }

        final File output = new File(tmp.getRoot(), "output.jar");

        final ShadeRequest request = new ShadeRequest();
        request.setJars(singleton(input));
        request.setUberJar(output);
        request.setResourceTransformers(emptyList());
        request.setRelocators(emptyList());
        request.setFilters(emptyList());
        request.setShadeSourcesContent(false);

        final DefaultShader shader = new DefaultShader();
        shader.shade(request);

        final Instant outInstant;
        try (final JarFile out = new JarFile(output)) {
            outInstant = out.getJarEntry("test.txt").getLastModifiedTime().toInstant();
        }
        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 point).

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.

[hboutemy]

expectation of this PR is Reproducible Builds
the test does not test reproducible builds, it tests what you think is the timestamp that should be in the zip, ignoring the fact that dos time in zip does not respect timezone
code in plexus-archiver works for Reproducible Builds = build twice, eventually in different timezones, and get the same binary zip at the end

one thing you can do is check what you get in a jar vs the project.build.outputTimestamp value that you defined in the pom.xml: you'll see an effect of this

[rmannibucau]

@hboutemy before testing reproducible builds you should test if you don't break any feature, as of today this code path was breaking timestamps so it must be fixed. As soon as you respect timestamps you are reproducible by design with the stand reproducible limitations - you must use the exact same jdk with the exact same env (which indeed includes the timezone - this is why i think this was a non-issue).
As of today timestamp consumer is not working as expected. Agree it was probably broken before when the "if" was introduced but also means keep this code path is highly broken and as of today I'm more keen to drop this condition than keep your merge for that reason - which means reproducible builds are still a thing but behavior is closer to the shade contract.

[hboutemy]

fyi https://issues.apache.org/jira/browse/MJAR-300 dived recently on DOS date in zip files that are not timezone aware vs Unix timestamp that are timezone aware
this is what plexus-archiver trick solves by modifying the zip entry timestamp to be binary stable = what I had to do again in the current shade case

[hboutemy]

test done with Reproducible Central: https://github.com/hboutemy/reproducible-central/tree/MSHADE-471/MSHADE-471

tests that using UTC does not any more get a different result to using a TZ with a DST

proves that the PR fixes the issue
sadly I don't see how to change this test (that uses Docker and a lot of side artifacts) into an IT in maven-shade-plugin
but no reason there will be a regression in the future

[rmannibucau]

• reproducible tests don't check the time is accurate whatever the timezone is (it is the opposite as shown by the test)
• UTC tests do not use UTC time so they are iso other tests as of today
• proves the time management is wrong in shade dependency inclusion feature which creates issues so we need to revisit it either enabling to configure the timezone per dependency or using the UTC time when it is available (indeed it does not enable reproducible builds if you build under another timezone by design but it is expected IMHO since otherwise code depending on jar resources time are just broken)

[hboutemy]

disagree: tests show reproducible builds independent from TZ = what they are supposed to be

you are looking for something else: detail what you are looking for and work on it if you want
that will be another Jira issue, another PR, another review