Fix pom.model properties read for bundle-projects and add it-tests #1090

HannesWell · 2022-06-28T23:00:06Z

The TychoBundleMapping tried to look up the buiild.properties of a 'eclipse-plugin' project in its META-INF folder instead of the project root and therefore made it impossible to set pom.model attributes as described in the Tycho-wiki, at least from the build.properties in the project root.

Add extensive integration test to ensure all pom.model attributes are correctly read
Remove DefaultTychoResolver.setBuildProperties() because its functionality is already provided by AbstractTychoMapping and it is currently not working. It seems to have operated on another project and its properties than those that are used later during execution.

github-actions · 2022-06-29T00:08:33Z

Unit Test Results

270 tests 266 ✔️ 45m 58s ⏱️
156 suites     4 💤
156 files     0 ❌

Results for commit a9eaf32.

♻️ This comment has been updated with latest results.

laeubi · 2022-06-29T04:55:21Z

Thanks Hannes for looking into this, about the DefaultTychoResolver I think the idea was to also use this in non-pomless builds, but I think we can remove this as it never was functional.

laeubi

This looks good and the enhanced test-cases are great, just a little "code-move" so we only address the properties here.

laeubi · 2022-06-29T05:03:03Z

tycho-extras/tycho-pomless/src/main/java/org/eclipse/tycho/pomless/AbstractTychoMapping.java

@@ -162,6 +161,10 @@ protected File getRealArtifactFile(File polyglotArtifactFile) {
        return polyglotArtifactFile;
    }

+    File getArtifactDirectory(Map<String, ?> options) {


I think we should not use this method here in the abstract class as on some places as we specifically want to support "directory poms". instead I think it would be better to move Reading-code of the build properties into getEnhancementProperties and instead have a (non static) getBuildProperties method that returns the location of the file.

The problem is that getBuildProperties(File) is used in multiple other places (i.e.. findParent() or isTestBundle()) where the project root file is at hand and that cannot use getEnhancementProperties().
Therefore in the end I think that this will make it more complicated.

If you think that this method is not suitable for the read() methods I can revert that, as I said I never hit that code and are therefore not certain that part of the change is good.

HannesWell · 2022-06-29T07:00:35Z

Thanks Hannes for looking into this, about the DefaultTychoResolver I think the idea was to also use this in non-pomless builds, but I think we can remove this as it never was functional.

Great. Additionally I think it is not the right way if to add Maven-properties via build.properties when there is a 'real' pom.xml that can have a properties section. In that case I would expect the pom to just work as usual without properties coming from other places like in a Maven build without Tycho.

This looks good and the enhanced test-cases are great [...]

Thanks. Yes after my first attempt to fix this also broke the reading I convinced myself to better have a good test coverage.
And I learned about the tycho.pomless.parent which is super useful and was the missing part for me to be able to remove some more poms in Equinox.

laeubi · 2022-06-29T09:47:50Z

@HannesWell if you could apply the suggested change so we d not change "too much" I think this is ready to be merged and backported to 2.x

HannesWell · 2022-06-29T10:10:46Z

@HannesWell if you could apply the suggested change so we d not change "too much" I think this is ready to be merged and backported to 2.x

I would like to do that, but as said in my reply to your remark I'm struggeling to apply it.
Nevertheless I can revert the change in the read methods.

laeubi · 2022-06-29T10:12:38Z

Could it be that you have not yet submit your reply? I don't see one here :-D

HannesWell

Indeed I forgot to submit my own review/replay. 😅
Sorry for that.

HannesWell · 2022-06-28T23:00:47Z

tycho-extras/tycho-pomless/src/main/java/org/eclipse/tycho/pomless/AbstractTychoMapping.java

@@ -108,8 +108,7 @@ public ModelWriter getWriter() {

    @Override
    public Model read(InputStream input, Map<String, ?> options) throws IOException, ModelParseException {
-        String location = PolyglotModelUtil.getLocation(options);
-        File file = new File(location);
+        File file = getArtifactDirectory(options);


Here I'm uncertain that this is correct in case of a bundle-project. But I never hit this code-path and therefore cannot check it.

HannesWell · 2022-06-28T23:01:07Z

tycho-extras/tycho-pomless/src/main/java/org/eclipse/tycho/pomless/AbstractTychoMapping.java

@@ -134,7 +133,7 @@ public Model read(File input, Map<String, ?> options) throws IOException, ModelP

    @Override
    public Model read(Reader input, Map<String, ?> options) throws IOException, ModelParseException {
-        File file = new File(PolyglotModelUtil.getLocation(options));
+        File file = getArtifactDirectory(options);


As above, I'm uncertain that this is correct at this place. But I never hit this code-path and therefore cannot check it.

tycho-its/src/test/java/org/eclipse/tycho/test/buildextension/PomlessTest.java

HannesWell · 2022-06-29T06:54:38Z

tycho-extras/tycho-pomless/src/main/java/org/eclipse/tycho/pomless/AbstractTychoMapping.java

@@ -162,6 +161,10 @@ protected File getRealArtifactFile(File polyglotArtifactFile) {
        return polyglotArtifactFile;
    }

+    File getArtifactDirectory(Map<String, ?> options) {


The problem is that getBuildProperties(File) is used in multiple other places (i.e.. findParent() or isTestBundle()) where the project root file is at hand and that cannot use getEnhancementProperties().
Therefore in the end I think that this will make it more complicated.

If you think that this method is not suitable for the read() methods I can revert that, as I said I never hit that code and are therefore not certain that part of the change is good.

laeubi · 2022-06-29T10:43:50Z

tycho-extras/tycho-pomless/src/main/java/org/eclipse/tycho/pomless/AbstractTychoMapping.java

@@ -277,8 +280,7 @@ protected static Properties getBuildProperties(File dir) throws IOException {

    @Override
    public Properties getEnhancementProperties(Map<String, ?> options) {
-        String location = PolyglotModelUtil.getLocation(options);
-        File file = new File(location);
+        File file = getArtifactDirectory(options);


What do you think about something like:

@Override public Properties getEnhancementProperties(Map<String, ?> options) { String location = PolyglotModelUtil.getLocation(options); File file = new File(location); try { return getEnhancementProperties(file); } catch (IOException e) { logger.warn("reading EnhancementProperties encountered a problem and was skipped for this reason", e); } return null; } protected Properties getEnhancementProperties(File file) throws IOException { if (file.isDirectory()) { return getBuildProperties(file); } return getBuildProperties(file.getParentFile()); }

and then we override in TychoBundleMapping:
Properties getEnhancementProperties(File file)
?

That's a good suggestion. Thank you. I applied it!

I also thought about simply moving the isDirectory()/isFile() logic into getBuildProperties() and then overwriting that in TychoBundleMapping, that would have worked for all other callers of getBuildProperties() in AbstractTychoMapping, but when being overridden in TychoBundleMapping it would break the other callers since getEnhancementProperties() is the only affected caller of that method. So this is a suitable solution.

HannesWell · 2022-06-29T19:37:36Z

I still suspect that the two read() methods that call PolyglotModelUtil.getLocation(options) actually suffer the same problem. But I never hit them and therefore assume that they are actually never called, or at least called in other scenarios I than those I tested.

But since the relevant scenarios I can think of are covered I think that should be OK for now. But we should keep this mind if problems in this area occur.

The TychoBundleMapping tried to look up the buiild.properties of a 'eclipse-plugin' project in its 'META-INF' folder instead of the project root. This change fixes that. + Add extensive integration tests to ensure all pom.model attributes are correctly read + Remove DefaultTychoResolver.setBuildProperties() because its functionality is already handled by AbstractTychoMapping and it is currently not working

HannesWell · 2022-06-29T19:44:24Z

As soon as we have merged this I can create PR against the 2.7.x branch with a back-port of this commit.

laeubi · 2022-06-30T05:14:19Z

But I never hit them and therefore assume that they are actually never called,

The interface contains some methods that are actually not called by the polyglot manager, so we should not care to much unless it causes problems :-)

The TychoBundleMapping tried to look up the buiild.properties of a 'eclipse-plugin' project in its 'META-INF' folder instead of the project root. This change fixes that. + Add extensive integration tests to ensure all pom.model attributes are correctly read + Remove DefaultTychoResolver.setBuildProperties() because its functionality is already handled by AbstractTychoMapping and it is currently not working Backport of commit: 4c2db22 See eclipse-tycho#1090

HannesWell · 2022-06-30T05:46:06Z

But I never hit them and therefore assume that they are actually never called,

The interface contains some methods that are actually not called by the polyglot manager, so we should not care to much unless it causes problems :-)

Yep. If we want to be sure those methods are not used accidentally, they could throw an UnsupportedOperationException. In case it is used one day at least we would notice immediately and not use a maybe or maybe not correctly working method.

Anyway, I did the backport of this in #1094.

The TychoBundleMapping tried to look up the buiild.properties of a 'eclipse-plugin' project in its 'META-INF' folder instead of the project root. This change fixes that. + Add extensive integration tests to ensure all pom.model attributes are correctly read + Remove DefaultTychoResolver.setBuildProperties() because its functionality is already handled by AbstractTychoMapping and it is currently not working Backport of commit: 4c2db22 See eclipse-tycho#1090

The TychoBundleMapping tried to look up the buiild.properties of a 'eclipse-plugin' project in its 'META-INF' folder instead of the project root. This change fixes that. + Add extensive integration tests to ensure all pom.model attributes are correctly read + Remove DefaultTychoResolver.setBuildProperties() because its functionality is already handled by AbstractTychoMapping and it is currently not working Backport of commit: 4c2db22 See #1090

HannesWell requested a review from laeubi June 28, 2022 23:00

laeubi reviewed Jun 29, 2022

View reviewed changes

HannesWell commented Jun 29, 2022

View reviewed changes

laeubi reviewed Jun 29, 2022

View reviewed changes

HannesWell force-pushed the fixPomModelProperties branch from 4f34e3e to eecbb1e Compare June 29, 2022 19:25

HannesWell force-pushed the fixPomModelProperties branch from eecbb1e to a9eaf32 Compare June 29, 2022 19:38

laeubi merged commit 4c2db22 into eclipse-tycho:master Jun 30, 2022

HannesWell deleted the fixPomModelProperties branch June 30, 2022 05:33

HannesWell mentioned this pull request Jun 30, 2022

[Backport 2.7.x] Fix pom.model properties read for bundle-projects and add it-tests #1094

Merged

HannesWell added this to the 2.7.4 milestone Jul 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pom.model properties read for bundle-projects and add it-tests #1090

Fix pom.model properties read for bundle-projects and add it-tests #1090

HannesWell commented Jun 28, 2022

github-actions bot commented Jun 29, 2022 •

edited

Loading

laeubi commented Jun 29, 2022

laeubi left a comment

laeubi Jun 29, 2022

HannesWell Jun 29, 2022

HannesWell commented Jun 29, 2022

laeubi commented Jun 29, 2022

HannesWell commented Jun 29, 2022

laeubi commented Jun 29, 2022

HannesWell left a comment

HannesWell Jun 28, 2022

HannesWell Jun 28, 2022

HannesWell Jun 29, 2022

laeubi Jun 29, 2022

HannesWell Jun 29, 2022

HannesWell commented Jun 29, 2022

HannesWell commented Jun 29, 2022

laeubi commented Jun 30, 2022

HannesWell commented Jun 30, 2022

Fix pom.model properties read for bundle-projects and add it-tests #1090

Fix pom.model properties read for bundle-projects and add it-tests #1090

Conversation

HannesWell commented Jun 28, 2022

github-actions bot commented Jun 29, 2022 • edited Loading

Unit Test Results

laeubi commented Jun 29, 2022

laeubi left a comment

Choose a reason for hiding this comment

laeubi Jun 29, 2022

Choose a reason for hiding this comment

HannesWell Jun 29, 2022

Choose a reason for hiding this comment

HannesWell commented Jun 29, 2022

laeubi commented Jun 29, 2022

HannesWell commented Jun 29, 2022

laeubi commented Jun 29, 2022

HannesWell left a comment

Choose a reason for hiding this comment

HannesWell Jun 28, 2022

Choose a reason for hiding this comment

HannesWell Jun 28, 2022

Choose a reason for hiding this comment

HannesWell Jun 29, 2022

Choose a reason for hiding this comment

laeubi Jun 29, 2022

Choose a reason for hiding this comment

HannesWell Jun 29, 2022

Choose a reason for hiding this comment

HannesWell commented Jun 29, 2022

HannesWell commented Jun 29, 2022

laeubi commented Jun 30, 2022

HannesWell commented Jun 30, 2022

github-actions bot commented Jun 29, 2022 •

edited

Loading