Add output entry to build.properties in o.e.help.base

[fedejeanne]

This should fix the build.

See: eclipse-tycho/tycho#3019

[github-actions]

Test Results

     591 files  +   438       591 suites  +438   1h 2m 29s ⏱️ + 13m 19s
  3 837 tests +1 035    3 832 ✔️ +1 037    5 💤 ±0  0 ❌  - 2 
12 117 runs  +3 105  12 081 ✔️ +3 107  36 💤 ±0  0 ❌  - 2 

Results for commit a71ae5b. ± Comparison against base commit b345854.

♻️ This comment has been updated with latest results.

[fedejeanne]

Test failure documented in #863

[HeikoKlare]

I am not sure if it makes sense to selectively add that configuration to single bundles. It does not only affect the help bundle. That is only the bundle for which the API validation fails in every build. Whether or not other bundle validations fail as well depend on whether they have changed in the PR. For example, see https://github.com/eclipse-platform/eclipse.platform/actions/runs/6906066723/job/18790238000

In my opinion, we should either fix all configurations or wait for Tycho 4.0.5, which should fix that issue and will be available one 4.31 development starts 8eclipse-platform/eclipse.platform.releng.aggregator#1530), and until then only selectively adapt configuration in case we need to make changes to a bundle for 4.30.

[fedejeanne]

According to this comment from @laeubi , Tycho makes an "opinionated fix" but the actual problem is that the configuration is missing an output folder which means the configuration is wrong. I'd say we merge this one and then start fixing other configurations regardless of whether or not Tycho can deal with them.
It's a pity that we are on freeze period now and we can't merge this right away. If we could, maybe the next build would fail and show the next plugin with a wrong configuration.

[HeikoKlare]

I'd say we merge this one and then start fixing other configurations regardless of whether or not Tycho can deal with them.

I have to admit that I do not understand the idea. Of course we should fix the configuration, but why should we follow a "trial and error" approach? The problem is known, so it can be fixed systematically instead of making one PR after another because a build fails. In particular, that's pretty annoying for new contributors, as they have no idea why their checks fails (which is already bad due to random failing tests but even worse for known configuration issues). As far as I understand, we just have to explicitly define an output for every source definition in the build.properties.

With respect to this PR, from my understanding it is even imcomplete for org.eclipse.help.base. This plugin defines two source folders but the PR only adds an ouput for one of them. Is it correct to leave out the one for ant_tasks/helpbase-ant.jar?

[fedejeanne]

I have to admit that I do not understand the idea. Of course we should fix the configuration, but why should we follow a "trial and error" approach?

We shouldn't, that's not what I meant. If there were a better approach to find all inconsistent configurations then we should use that and fix them all at once. But since there's none (that I know about) then one could at least use the failing builds to identify some of them one by one. My fear is that Tycho is not actually fixing the error but just hiding it by using some default values.

With respect to this PR, from my understanding it is even imcomplete for org.eclipse.help.base. This plugin defines two source folders but the PR only adds an ouput for one of them. Is it correct to leave out the one for ant_tasks/helpbase-ant.jar?

Good point. I think in this specific case it might be fine not to include it because:

• I haven't find any usage of it in my WS and
• It's under an internal package (so other clients should not use it either) and
• Its package (org.eclipse.help.internal.base.ant) is not being exported in the MANIFEST so it seems to be considered "non existing for the outside world"

But that's only my opinion. Maybe @laeubi or @jukzi can provide some insight here?

@laeubi what would happen if one doesn't specify anything under output..? Would Tycho only add the default folder (bin) to the output or would it also add any other folders (which ones)?

[laeubi]

Good question, best to find out by a running the maven build and look at the target folder what classes are located in what folders. The "library" concept of PDE is quite odd an can only be partly mapped to maven but we try our best.

[jukzi]

Maybe @laeubi or @jukzi can provide some insight here?

no, i am only victim in this case. It would really help if tycho would just just the default as is did for ages @akurtakov

[HeikoKlare]

We shouldn't, that's not what I meant. If there were a better approach to find all inconsistent configurations then we should use that and fix them all at once. But since there's none (that I know about) then one could at least use the failing builds to identify some of them one by one.

Why shouldn't it be possible to scan the build.properties files for missing output specifications?

Good point. I think in this specific case it might be fine not to include it because:

• I haven't find any usage of it in my WS and
• It's under an internal package (so other clients should not use it either) and
• Its package (org.eclipse.help.internal.base.ant) is not being exported in the MANIFEST so it seems to be considered "non existing for the outside world"

That's not correct. The helpbase-ant.jar is part of the generated plugin jar and placed in the ant_tasks folder. The jar is put on the classpath for Ant tasks via an extension point and the BuildHelpIndex class from this jar is provided as an ant task via an extension point in the org.eclipse.help.base plugin. I guess the reason why the package is internal is that you should not refer to this class but use the ant task provided as an extension, which then (internally) uses that class.

[HeikoKlare]

Good question, best to find out by a running the maven build and look at the target folder what classes are located in what folders. The "library" concept of PDE is quite odd an can only be partly mapped to maven but we try our best.

I made some tests and for the generated artifacts it makes no difference. Tycho seems to generate the classes to folders according to what is specified in the source entries in the build.properties, i.e., it uses target/classes for those in source.. and target/X for those in source.X. In this case, the generated files for the sources of source.ant_tasks/helpbase-ant.jar are placed in target/ant_tasks/helpbase-ant.jar.

The problem then arises with the API tools, as they use the output folders specified on the Eclipse Java project classpath. These folder are empty if the sources have not been compiled in the IDE, which places the generated class files exactly there. Adding output.X=Y ensures that the files are copied to Y for source X, so that the API tools find them. Since there is not public API in the ant_tasks/helpbase-ant.jar source, it does not matter whether a proper output is specified for that source. So my expectation is that the missing configuration of an output for ant_tasks/helpbase-ant.jar will only lead to problems with API tools once you add public API to that source folder.

I have to admit that I find it a little weird that Tycho compiles to a different folder than the folder used by Tycho API tools for validation, but I guess there is some reason for that which I do not know about, as I have not followed all the discussion on this topic at Tycho.
In general, I do not understand why we let Tycho compile to a different folder than the IDE at all, i.e., why the Tycho output folders do not conform to the ones defined for the Java project. I would understand that if you separate IDE and Tycho builds to not mix up the generated class file, but since we have to configure Tycho to copy the generated class files to where the IDE generates its files, it does not seem to make a difference.

[merks]

If you ever do a Tycho build on the same folders as are being used in your IDE, you will not be happy with the result if they overwrite each other. I do this regularly and for some projects it's horrible that one has to do a clear build and hope it really gets rebuilt in the IDE properly...

[laeubi]

I have to admit that I find it a little weird that Tycho compiles to a different folder than the folder used by Tycho API tools for validation, but I guess there is some reason for that which I do not know about

The reason is that no one really likes to maintain API tools, but API tools requires to use an Eclipse Project and then makes assumptions about locations that are not valid inside the maven reactor, especially these issues are relevant in that regards:

• [apitools] Use the AST/JDT to discover package names eclipse-pde/eclipse.pde#801
• [apitools] no API error reported with missing bin folder eclipse-pde/eclipse.pde#791
• [apitools] BaseApiAnalyzer should have a way to discover the source without a java project eclipse-pde/eclipse.pde#785

This all is work in progress and we improve it whenever possible (and there are already mitigations/patches available at Tycho) but as mentioned earlier Platform is in freeze period until master opens so we can not apply them NOW, and Tycho can't release until Eclipse is GA to update to never constructs in API tools as we never publish official artifacts to maven before.

So instead of crying for help from @akurtakov or complaining about the bad world in general if one is really concerned about the general thing, it would be better to participate (and help with) these efforts

• Restructure releng work eclipse.platform.releng.aggregator#1472
• [Proposal] Switch master to next release after M3 eclipse.platform.releng.aggregator#863
• Deploy the milestones (and maybe Releasecandidates) at maven central eclipse.platform.releng.aggregator#1002
• Feed the next streams update site repository with the contents of M1 / M2 / RC1 / RC2 prior to release eclipse.platform.releng.aggregator#884
• ... and similar ...

so we get rid of these "freezes", can release stuff more often, having Milestones at maven for faster feedback and adoption...

[fedejeanne]

That's not correct. The helpbase-ant.jar is part of the generated plugin jar and placed in the ant_tasks folder. The jar is put on the classpath for Ant tasks via an extension point and the BuildHelpIndex class from this jar is provided as an ant task via an extension point in the org.eclipse.help.base plugin. I guess the reason why the package is internal is that you should not refer to this class but use the ant task provided as an extension, which then (internally) uses that class

Thank you, I missed that one.

Why shouldn't it be possible to scan the build.properties files for missing output specifications?

You mean with a regex pattern? I'm not an expert in regex so best case scenario I could only find plugins without any output entry, but not those for which only some output entries are missing.

@laeubi I have to admit I haven't been following those issues/discussions in detail, thank you for pointing them out! I'll try to take a look at them in the coming days :-)

[HeikoKlare]

Thanks for all the input, @laeubi. And please don't get me wrong, I did not want to blame anything or anyone. I just tried to understand at this example what happens and tried to share my understanding, as I have the impression that I am not the only one lacking understanding to decide on what we should do with the configurations. And since I did not totally understand the reasons for some of the behavior or configurations, I just shared my thoughts about that.

Even with the API tools improvement to be merged after the freeze period, it still seems to make sense to me to improve the build.properties in terms of having proper output specifications to reflect what is defined in the projects classpath, doesn't it? Some projects already have that, so it would make sense to me to make this consistent.

You mean with a regex pattern? I'm not an expert in regex so best case scenario I could only find plugins without any output entry, but not those for which only some output entries are missing.

Actually, I would just scan through the platform repo projects mainly by hand and fix everything related to the output folders at once (project classpath conforming to build.properties and consistent build.properties in itself) to hopefully have a proper configuration everywhere. I can try to provide a PR for that.

[fedejeanne]

Even with the API tools improvement to be merged after the freeze period, it still seems to make sense to me to improve the build.properties in terms of having proper output specifications to reflect what is defined in the projects classpath, doesn't it? Some projects already have that, so it would make sense to me to make this consistent.

I think that too. It would make sense to me if the build properties are written in such a way that exporting the plugin from Eclipse and building it with Tycho produce "the same" results... ("the same" unless of course Tycho needs to do something special and forcibly add something to the output, but I hope this is not the case).

Actually, I would just scan through the platform repo projects mainly by hand ...

In light of everything I learned in this discussion: this seems like the best option since the sole fact that Tycho does not fail the building process doesn't seem to mean that the configuration is complete (since Tycho makes assumptions). And please don't get me wrong either: I'm not judging Tycho, I just like my tools to make as little assumptions as possible so I don't loose sight of the overall process (my personal taste).

[HeikoKlare]

Improvement of build.properties for this repo provided in: #871

It would make sense to me if the build properties are written in such a way that exporting the plugin from Eclipse and building it with Tycho produce "the same" results...

Just for clarification: they already do that right now. The issue does not concern the build output in terms of the generated artifacts but only the API validation. In case you just perform a Tycho build without further settings, everthing will work as expected as API baseline comparison is skipped by default. And in case you already did an IDE build on the sources and then execute a Tycho build even with API baseline comparison, it will also work because the artifacts used for comparison already exist at the expected places due to the IDE build.

[fedejeanne]

Thank you for the PR and the clarification!

[fedejeanne]

I just pushed the missing entry. Thank you @HeikoKlare for pointing it out!

[iloveeclipse]

Required for #869