[bndtools classpath] -testpath deps not flagged as test if testsrc folder isn't flagged as test

[kriegfrj]

I am in the process of writing a few regression tests to test the behaviour of the Bnd Classpath Container in Bndtools. This is to minimise the chances of inadvertently breaking something while implementing #3175.

In the process, I have discovered that dependencies specified in -testpath are not automatically marked as "test only" dependencies unless the project also has a test source directory configured. (Specifically, the Eclipse project - even if the Bnd project has a test source directory but for some reason this has not been configured in the Eclipse project, it still won't flag the dependencies as test-only dependencies).

This means that, in Eclipse with Bndtools, dependencies on -testpath will still be visible to code in (non-test) source if you don't have a test folder configured.

It looks as if this behaviour has been deliberately written in there. If so, I'm curious to understand the reasoning as to me this doesn't seem correct - dependencies on -testpath should not be visible to non-test source.

I guess we're trying to maintain build fidelity so so the right question to ask is: what do the other drivers do?

[bjhargrave]

It looks as if this behaviour has been deliberately written in there.

Do you have a pointer to this I can look at? I probably wrote the code which does this.

[kriegfrj]

https://github.com/bndtools/bnd/blob/5a3beb66a465ba4733456ef86a2d325562a319c2/bndtools.builder/src/org/bndtools/builder/classpath/BndContainerInitializer.java#L329-L337

The testattr flag is only true if calculateContainersClasspath() is called for -testpath and there is a test source package fragment root configured in the Eclipse project.

[bjhargrave]

That code says, find if there is any source entry in the container which has the test classpath attribute.

See https://github.com/bndtools/bnd/commit/273d90b82344965fb7f97b877c4db137a3ab8a94

So this was specifically done. I suspect that if there were no source entries marked with the test classpath attribute, then it was not a good thing to mark other classpath entries as test or without_test_code.

It seems a strange project configuration to have no test source folder and yet have -testpath configured.

[kriegfrj]

One possibility occurred to me: I remember that test package roots are a relatively new addition to Eclipse. The above code may have been a way of selectively enabling the test attribute if it is supported by the version of Eclipse that is running.

If this is correct, then given that we are now running at a higher Eclipse baseline where test package roots will be guaranteed to be supported, then maybe this filter is now legacy code and can be removed - we just set testattr = instruction.equals(Constants.TESTPATH).

[kriegfrj]

That code says, find if there is any source entry in the container which has the test classpath attribute.

Minor nit - the code says, find if there is any source entry ~in the container~on the raw classpath which has the test classpath attribute. The raw classpath is the unexpanded list of entries in .classpath. This is usually where the test source directory would be configured.

So this was specifically done. I suspect that if there were no source entries marked with the test classpath attribute, then it was not a good thing to mark other classpath entries as test or without_test_code.

Thanks to your link to the specific commit, I managed to dig up the related issues and PRs which shed more light on why this was done:

bndtools/bndtools#1902 bndtools/bndtools#1903 bndtools/bndtools#1904 bndtools/bndtools#1905 bndtools/bndtools#1906 bndtools/bndtools#1907

https://github.com/bndtools/bndtools/wiki/Changes-in-4.1.0#backwards-compatibility

It seems that my guess above was close to the mark - bndtools/bndtools#1906 describes the legacy situation this code was put into place to allow for.

It seems a strange project configuration to have no test source folder and yet have -testpath configured.

Note that it's not that the Bnd project doesn't have a test source folder, but that the corresponding IClasspathEntry in the raw Eclipse .classpath may not have had the test attribute set. Suddenly marking the test attribute of test dependencies on -testpath had the effect of making the test dependencies invisible to the source in these legacy test source folders.

Automatic project synchronization would be a better way of taking care of this, I think - instead of making allowances for strange (Eclipse) project configurations, it would directly fix the strange Eclipse project configuration. Failing that, to generate a warning or error if the test source directory doesn't have the test=true attribute set. Maybe we can look at removing this code once one of these two options is in place.

[bjhargrave]

I am not sure what is remaining here.

[kriegfrj]

I believe that enough time has passed that the special case should be removed and -testpath should always be flagged as a test classpath entry. It is awkward and leads to unexpected behaviour. In the off chance anyone still has a project out there that's configured this way, backward compatibility could be maintained by a project sync mechanism which adds the test flag to the test source dir.

[bjhargrave]

I believe that enough time has passed that the special case should be removed

I am not sure that it is a special case. If none of the test folders are marked test=true then marking the -testpath entries as test=true will mean the test code will not compile.

a project sync mechanism which adds the test flag to the test source dir.

This can be done as a "quick fix" or something, but that does not mean we should remove the handling for the case where the test folders are not marked test=true. There may be old source trees that are this way.

[kriegfrj]

I believe that enough time has passed that the special case should be removed

I am not sure that it is a special case. If none of the test folders are marked test=true then marking the -testpath entries as test=true will mean the test code will not compile.

In ordinary (modern) usage, such a situation should not be considered the norm. There is no good reason for a project to be configured this way. Projects that are configured this way obliterate the distinction between -testpath and -buildpath, which consequently means that they break build fidelity with all other Bnd drivers. If it should not be considered the norm, then by definition it is a special case.

a project sync mechanism which adds the test flag to the test source dir.

This can be done as a "quick fix" or something, but that does not mean we should remove the handling for the case where the test folders are not marked test=true. There may be old source trees that are this way.

Understand that there may be old source trees that are this way and I agree that we need to accommodate them - however, I think it is better to handle them by helping them to fix the error and modernise their project configurations, rather than by silently allowing the error to persist and allowing these badly configured projects to remain so indefinitely.

I propose this as the way forward:

• For 6.3:
  • Deprecate support for this sort of project configuration;
  • Update the BndtoolsBuilder to generate a warning marker when it detects projects that are configured this way.
  • Tidy-up and re-instate the project sync mechanism to automatically fix this by adding the test=true attribute to all the classpath entries configured for testsrc entries.
• For 7.0:
  • Remove support for this sort of project configuration.
  • Upgrade the warning marker to an error marker.

Thoughts?

[bjhargrave]

I think some of this also depends upon what Eclipse m2e and the gradle eclipse plugin generate for .classpath. But I would assume their current versions do add the test attribute were appropriate.

[pkriens]

@kriegfrj any intentions to work on this in the forseeable future?

[kriegfrj]

Happy for it to be moved to abeyance.