Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move bnd-maven-plugin to a profile. #1665

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

motlin
Copy link
Contributor

@motlin motlin commented Aug 8, 2024

Moved bnd-maven-plugin configuration to the release-artifacts profile so that it will affect released jars but will otherwise not affect local development.

cc @fipro78

@motlin motlin requested a review from donraab August 8, 2024 13:06
@fipro78
Copy link
Contributor

fipro78 commented Aug 8, 2024

But that also means that you are not able to test the OSGi stuff with a local build, because the MANIFEST.MF files are not generated with the necessary OSGi meta-data. You need to do a release build, which you can not do locally IIUC. So how would you verify that the changes work in an OSGi environment with a local build after that change?

IMHO that change is unfortunate.

@motlin
Copy link
Contributor Author

motlin commented Aug 8, 2024

But that also means that you are not able to test the OSGi stuff with a local build. You need to do a release build, which you can not do locally IIUC. So how would you verify that the changes work in an OSGi environment with a local build after that change?

I created this PR somewhat as an experiment, not sure if the builds will pass. I think the fact that the builds pass shows that there are no automated tests of the OSGi functionality. What is your expectation for testing - manual or automated tests? I'm not aware of anyone doing manual testing either - am I missing anything?

@fipro78
Copy link
Contributor

fipro78 commented Aug 8, 2024

I suppose there are no automatic OSGi tests, as before the OSGi bundle was created in a separate step that created the p2 repository. Now the artifacts are itself OSGi bundles. But I have not added OSGi specific tests, would need to see how that would look like. So I was referring to manual tests, which I did a lot while I was implementing the changes. If you ever have an issue related to OSGi, nobody will be able to work in that area, because you will only get the OSGi meta-data on a release build. So it can only be fixed after a release.

It actually generates the meta-data. Why do you want it to be done only for a release and not already at development time? This way the release artifacts would be different in terms of the content from development artefacts.

@motlin
Copy link
Contributor Author

motlin commented Aug 8, 2024

If folks do want to do OSGi testing manually, I could arrange the maven profiles slightly differently to make that happen. Rather than reuse the release-artifacts profile, I'd create a new profile called bnd-maven-plugin that also is activated when performRelease is set to true. This way, anyone could test with --activate-profiles bnd-maven-plugin if they want, and we could even activate that profile in GitHub workflows if that gives peace of mind.

However, I suspect there are no manual tests of OSGi functionality of any sort. You know I was running into trouble getting the bnd annotations to be recognized by my IDE. After staring at the problem for a while I realized that it was running the maven build which broke the IDE, and running a clean build from the IDE which fixed it. That means whenever I'm running tests or debugging in the IDE, I'm doing so after clearing out everything related to OSGi.

There is probably another way forward here, which is to configure the IDE with annotation processors or whatever it needs in order to understand the OSGi metadata, but I don't know how.

@fipro78
Copy link
Contributor

fipro78 commented Aug 8, 2024

What setup do you have? Which IDE? What steps do you perform to raise the issue you have?

Maybe I can reproduce it somehow and find a better solution.

@motlin
Copy link
Contributor Author

motlin commented Aug 8, 2024

What setup do you have? Which IDE? What steps do you perform to raise the issue you have?

Maybe I can reproduce it somehow and find a better solution.

To reproduce:

git checkout upstream/master
mvn clean install

Open IntelliJ. Navigate to any test. As an example, I was just working on UnmodifiableMutableOrderedMapTest. Click the gutter icon that looks like a play button to run the test. The IDE will error out with many lines of the form:

/Users/Craig/projects/eclipse-collections/eclipse-collections-api/target/generated-sources/java/org/eclipse/collections/api/factory/primitive/IntStacks.java:24:22
java: package aQute.bnd.annotation.spi is not visible
  (package aQute.bnd.annotation.spi is declared in the unnamed module, but module org.eclipse.collections.api does not read it)

To get around this error, navigate to the Build menu, then Rebuild Project. This is the step that's like a clean and gets rid of all OSGi metadata. It takes a few minutes because it has to recompile everything.

Afterward, the tests will run. If we switch back to maven and run mvn install without clean then we can get maven to error with a sort of mirrored set of error messages.

10:22:12:654 [ERROR] /Users/Craig/projects/eclipse-collections/eclipse-collections-api/target/generated-sources/java/org/eclipse/collections/api/factory/primitive/LongLists.java:[24,21] error: package aQute.bnd.annotation.spi is not visible

@fipro78
Copy link
Contributor

fipro78 commented Aug 8, 2024

And why do you call mvn clean install and not just mvn clean verify?

@motlin
Copy link
Contributor Author

motlin commented Aug 8, 2024

And why do you call mvn clean install and not just mvn clean verify?

No reason, I think either one will reproduce the issue.

@fipro78
Copy link
Contributor

fipro78 commented Aug 8, 2024 via email

@motlin
Copy link
Contributor Author

motlin commented Aug 8, 2024

Have you tried the verify approach?

Just tried it now, same thing happens.

While scrolling through the error messages to make sure they are identical, I realized there's another pattern of error mixed in.

Same as before:

/Users/Craig/projects/eclipse-collections/eclipse-collections-api/target/generated-sources/java/org/eclipse/collections/api/factory/primitive/IntStacks.java:24:22
java: package aQute.bnd.annotation.spi is not visible
  (package aQute.bnd.annotation.spi is declared in the unnamed module, but module org.eclipse.collections.api does not read it)

The one I didn't notice/mention before:

/Users/Craig/projects/eclipse-collections/eclipse-collections-api/target/generated-sources/java/org/eclipse/collections/api/factory/primitive/IntStacks.java:25:26
java: package aQute.bnd.annotation.spi does not exist

@fipro78
Copy link
Contributor

fipro78 commented Aug 8, 2024

Hm, it looks like some issue with the generated jpms stuff. Maybe intellij uses the module.info to determine the test build module path.

Need to investigate in that area. There are posts related to similar issues in the internet. Maybe the spi packages need to be specified somehow although not necessary at runtime?

@fipro78
Copy link
Contributor

fipro78 commented Aug 8, 2024

@motlin
Copy link
Contributor Author

motlin commented Aug 8, 2024

To narrow down the issue, could you try to change the scope of the bnd annotations from provided to compile? Just to see if this changes anything.

I tried changing the scope of the dependency to compile in both places, reran a clean maven build, and clicked run on some tests in IntelliJ. I got the error messages about "java: package aQute.bnd.annotation.spi is not visible", just the one error kind this time.

@fipro78
Copy link
Contributor

fipro78 commented Aug 9, 2024

@motlin

I tried different IDEs to see if it is a general issue:

  1. Eclipse IDE
  • Fresh checkout from GitHub git clone https://github.com/eclipse/eclipse-collections.git
  • Perform a build as explained in the docs mvn clean install -DskipTests=true
  • Import projects

After the import I get several errors about missing classes. The generated sources in the target folder are not added as source folders!

  • Fix the project setups

    • manually add the target/generated-sources/java folder to the build path of eclipse-collections-api and eclipse-collections
    • create empty src/main/java and src/main/resources folders in the unit-tests project
    • add the target/generated-test-sources/java folder to the build path of the unit-tests project and configure it as test resource via properties
  • Run the UnmodifiableMutableOrderedMapTest via Run As - JUnit Test

Execution succeeds!

  • Rerun the Maven build mvn clean install -DskipTests=true
  • Run the UnmodifiableMutableOrderedMapTest via Run As - JUnit Test

Execution succeeds!

  1. VS Code
  • Fresh checkout from GitHub git clone https://github.com/eclipse/eclipse-collections.git
  • Perform a build as explained in the docs mvn clean install -DskipTests=true
  • Import projects

After the import I get several errors about missing classes. The generated sources in the target folder are not added as source folders!

  • Fix the project setups
    To add the generated sources as source folders you need to modify the pom.xml files and add the following to the plugins section of eclipse-collections-api and eclipse-collections after the eclipse-collections-code-generator-maven-plugin
<plugin>
	<groupId>org.codehaus.mojo</groupId>
	<artifactId>build-helper-maven-plugin</artifactId>
	<version>3.2.0</version>
	<executions>
		<execution>
			<phase>generate-sources</phase>
			<goals>
				<goal>add-source</goal>
			</goals>
			<configuration>
				<sources>
					<source>target/generated-sources/java</source>
				</sources>
			</configuration>
		</execution>
	</executions>
</plugin>

In the pom.xml file of the unit-tests project add the following plugin:

<plugin>
	<groupId>org.codehaus.mojo</groupId>
	<artifactId>build-helper-maven-plugin</artifactId>
	<version>3.2.0</version>
	<executions>
		<execution>
			<phase>generate-test-sources</phase>
			<goals>
				<goal>add-test-source</goal>
			</goals>
			<configuration>
				<sources>
					<source>target/generated-test-sources/java</source>
				</sources>
			</configuration>
		</execution>
	</executions>
</plugin>
  • Run the UnmodifiableMutableOrderedMapTest via Test Runner for the Java extension

Execution succeeds!

  • Rerun the Maven build mvn clean install -DskipTests=true
  • Run the UnmodifiableMutableOrderedMapTest via Test Runner for the Java extension

Execution succeeds!

  1. IntelliJ
  • Fresh checkout from GitHub git clone https://github.com/eclipse/eclipse-collections.git
  • Perform a build as explained in the docs mvn clean install -DskipTests=true
  • Import projects

No errors at this point. Probably because of the .idea folder or any other configuration.

  • Run the UnmodifiableMutableOrderedMapTest

Execution succeeds!

  • Rerun the Maven build mvn clean install -DskipTests=true
  • Run the UnmodifiableMutableOrderedMapTest

Execution fails with the java: package aQute.bnd.annotation.spi is not visible error!

IMHO it is a bug in IntelliJ.

  • The Maven build succeeds and other IDEs do not show this error.
  • The error itself is strange, because it says that the package is not visible, but the compilation succeeds. I can only assume that it is related to JPMS and that IntelliJ uses the module system for the compilation internally. The annotation library is not a module and therefore does not export its packages via module-info. So yes, the annotations are not visible for JPMS. But they don't have to, because they are not needed at runtime!

My best guess is that when the projects are initially opened, IntelliJ does not recognize the JPMS nature. After the build there is some hook or other mechanism that spots the module-info.class in the resulting JAR and makes the projects "module" projects, which then bring up the error. Which I don't really understand, because the project itself is not setup as a Java module project. The module nature is added at build time and only in the resulting JAR. Maybe the mvn install triggers some update in IntelliJ which then does a Maven Update and gets the information from the local Maven repository instead of the project. But that is just a guess, not sure if that is really the case. You could try to do the mvn install only for the code generator plugin, which is needed to be installed. Then only call mvn clean verify to avoid that the API and IMPL modules get installed into the local repository, and see if that changes anything. That of course means you first need to clear your local Maven repository to avoid that existing artefacts are loaded from there.

I am not an IntelliJ user, and therefore I have absolutely no idea what is going on here and why. Sorry to say that, but at this point I am not able to help out anymore. The code is correct, the project setup could be corrected to work with all IDEs (see above, the pom.xml changes should be sufficient for both, Eclipse and VS Code). Why IntelliJ behaves this way, no idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants