Quarkus: We do not handle existing MANIFEST.MF files properly

[stuartwdouglas]

See https://stackoverflow.com/questions/55124900/how-to-create-native-executable-using-quarkus#

In this case we should probably modify the existing manifest to add the Main-Class entry

[cescoffier]

Oh, that's the issue! But why did Eclipse try to create a MANIFEST.MF?

[cescoffier]

So, basically the logic would be to check for the existence of target/classes/META-INF/MANIFEST.MF and if there inject the classpath and main class.

[cescoffier]

(when I say inject, read replace if already set, with a warning)

[machi1990]

How about replacing the old file with a fresh copy on each build?

[mickaelistria]

I don't know why Eclipse IDE/m2e generates MANIFEST.MF files, but in any case, having some application builds that generate some MANIFEST.MF is not so unusual and this case should be embraced by Quarkus rather than rejected.
So indeed, the approach of merging MANIFEST.MF (and failing by default in case of conflicting directives in the MANIFEST.MF) seems to be the most sustainable and portable path.

[Riduidel]

From what I understand, it seems there various steps that are run :

1. AugmentPhase will ... augment various aspects ? (I don't know which ones)
2. RunnerJarPhase will generate from the augmented (?) sources the output jar.
   To my mind, the best way to cope with pre-existing MANIFEST.MF in target would be to add another implementation of AppCreationPhase
   What do you think of that (and particularly what @aloubyansky think about that) ?

[dmlloyd]

We probably want a build item (optional) for the input manifest, and something for the output. The key is to ensure that we don't replace the manifest in-place, as cumulative changes would prevent build repeatability.

[gsmet]

@dmlloyd I'm failing to see how that would prevent the IDE from generating its own MANIFEST.MF?

I think I would just update DevMojo and RunnerJarPhase to update a possibly existing MANIFEST with our entries (it's just the classpath and the main class). We could bail out (and either throw an error or log a warning) if they are already defined.

What would be interesting would be to have an example of this generated MANIFEST.MF so that we can see if it defines the classpath or main class.

@Riduidel would you like to give it a try?

[Riduidel]

@gsmet I would be very happy to attempt a solution as a PR. I will try to do that this week-end

[Riduidel]

Howevern strangely, quarkus dev mode works correctly, so it seems to me the dev mode is not impacted (but I would love to have a way to make sure dev mode is not impacted)

[Riduidel]

I'm quite confident now that I understand how RunnerJarPhase produces the manifest. Let me explain what I understand to see if I'm right.

So, in RunnerJarPhase#buildRunner(....) we have (in lines 330-350) that code

        final Manifest manifest = new Manifest();
        manifest.getMainAttributes().put(Attributes.Name.MANIFEST_VERSION, "1.0");
        manifest.getMainAttributes().put(Attributes.Name.CLASS_PATH, classPath.toString());
        manifest.getMainAttributes().put(Attributes.Name.MAIN_CLASS, mainClass);
        try (OutputStream os = Files.newOutputStream(runnerZipFs.getPath("META-INF", "MANIFEST.MF"))) {
            manifest.write(os);
        }

        copyFiles(augmentOutcome.getAppClassesDir(), runnerZipFs);
        copyFiles(augmentOutcome.getTransformedClassesDir(), runnerZipFs);

        for (Map.Entry<String, List<byte[]>> entry : services.entrySet()) {
            try (OutputStream os = Files.newOutputStream(runnerZipFs.getPath(entry.getKey()))) {
                for (byte[] i : entry.getValue()) {
                    os.write(i);
                    os.write('\n');
                }
            }
        }

What I understant is that quarkus first create its own manifest, then copy existing files. It explains how previously existing manifest will overwrite quarkus one (hence my bug). How to fix that ? I will move the manifest generation code in its own method, move call to that method after file copy and in that method add declarations to existing manifest instead of creating a new one.

Is it ok for you ?

@gsmet I took time to answer, but yes, I'll give it a try ;-)

[dmlloyd]

We should be modifying the manifest via build items IMO. Maybe extensions should produce manifest entry items, which are then merged into the final product.

[mickaelistria]

it could

1. copy the file
2. read manifest if existing, or create new one
3. alter manifest
4. copy it

Something like

        copyFiles(augmentOutcome.getAppClassesDir(), runnerZipFs);
        copyFiles(augmentOutcome.getTransformedClassesDir(), runnerZipFs);

        for (Map.Entry<String, List<byte[]>> entry : services.entrySet()) {
            try (OutputStream os = Files.newOutputStream(runnerZipFs.getPath(entry.getKey()))) {
                for (byte[] i : entry.getValue()) {
                    os.write(i);
                    os.write('\n');
                }
            }
        }

        final Manifest manifest = new Manifest();
        Path targetManifestPath = runnerZipFs.getPath("META-INF", "MANIFEST.MF"));
        if (Files.exists(targetManifestPath)) {
           try (InputStream stream = Files.newInputStream(targetManifestPath)) {
             manifest.read(stream);
           }
        }
        manifest.getMainAttributes().put(Attributes.Name.MANIFEST_VERSION, "1.0");
        manifest.getMainAttributes().put(Attributes.Name.CLASS_PATH, classPath.toString());
        manifest.getMainAttributes().put(Attributes.Name.MAIN_CLASS, mainClass);
        try (OutputStream os = Files.newOutputStream(targetManifestPath) {
            manifest.write(os);
        }

[Riduidel]

Well, I suggest you take a look at the pull request #1540 :-)