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

Delay classpath resolution to the compile phase #460

Closed
laeubi opened this issue Dec 31, 2021 · 32 comments
Closed

Delay classpath resolution to the compile phase #460

laeubi opened this issue Dec 31, 2021 · 32 comments
Assignees
Milestone

Comments

@laeubi
Copy link
Member

laeubi commented Dec 31, 2021

Currently classpath resolution is performed as part of the project setup in the TychoResolver.resolveProject(MavenSession, MavenProject, List<ReactorProject>) this has the disadvantage that

  • it is probably done unnecessarily (e.g. if I call mvn validate and probably mvn clean)
  • we need to handle parallel builds while otherwise maven would do the job for us
  • it is not possible to consider sources for the class-path that are part of the regular maven build (might be related to this discussion Run plugins before Maven-Tycho build #434)
  • it is not possible to consider non-tycho projects that provide additional artifacts (eg. from felix-bundle-plugin)

looking at the code of OsgiBundleProject.resolveClassPath(MavenSession, MavenProject) it seems not strictly necessary to run this code at this early phase...

The result of this operation is set into the following keys:

  • CTX_ECLIPSE_PLUGIN_CLASSPATH
  • CTX_ECLIPSE_PLUGIN_STRICT_BOOTCLASSPATH_ACCESSRULES
  • CTX_ECLIPSE_PLUGIN_BOOTCLASSPATH_EXTRA_ACCESSRULES

interestingly these are only used inside OsgiBundleProject and/or Tycho compiler mojo but not accessed at the early stage, so it seems valid to init them when first accessed.

@laeubi laeubi self-assigned this Dec 31, 2021
laeubi added a commit to laeubi/tycho that referenced this issue Dec 31, 2021
Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
@laeubi laeubi closed this as completed in 4980ba6 Dec 31, 2021
@cdietrich
Copy link
Contributor

cdietrich commented Jan 1, 2022

this seems to break Xtext builds
classes produced by xtend-maven-plugin seem to be no longer found
https://github.com/itemis/xtext-reference-projects/runs/4678243989?check_suite_focus=true

git clone git@github.com:itemis/xtext-reference-projects.git
cd xtext-reference-projects/greetings-tycho/2.26.0-J11/org.xtext.example.mydsl.parent/
mvn clean install -Dtycho-version=2.6.0-SNAPSHOT

@laeubi
Copy link
Member Author

laeubi commented Jan 1, 2022

@cdietrich thanks for reporting the issue, it seems your use-case is currently not covered by any integration test do you think you can derive a minimal example?

@laeubi
Copy link
Member Author

laeubi commented Jan 1, 2022

By the way I see these warnings are they expected?

Warning: Could not transfer metadata org.antlr:antlr-runtime/maven-metadata.xml from/to maven-default-http-blocker (http://0.0.0.0/): transfer failed for http://0.0.0.0/org/antlr/antlr-runtime/maven-metadata.xml

WARN StandaloneSetup - No project file found for /home/runner/work/xtext-reference-projects/xtext-reference-projects/greetings-tycho/2.26.0-J11/org.xtext.example.mydsl/target/classes. The folder was neither an Eclipse 'bin' folder, nor a Maven 'target/classes' folder, nor a Gradle bin/main folder

[INFO] skip compiling sources because the configured directory '[/home/runner/work/xtext-reference-projects/xtext-reference-projects/greetings-tycho/2.26.0-J11/org.xtext.example.mydsl/src/main/java]' does not exist.

@cdietrich
Copy link
Contributor

i am not sure if you want to introduce a (circular) dependency to xtend in your tests.
the warnings should not be a problem. the are also there with 2.5

@laeubi
Copy link
Member Author

laeubi commented Jan 1, 2022

i am not sure if you want to introduce a (circular) dependency to xtend in your tests.

As long as everything is available in maven central I don't see a problem if we can improve the test-converage.

@laeubi
Copy link
Member Author

laeubi commented Jan 1, 2022

the warnings should not be a problem. the are also there with 2.5

Can you tell if the build order has changed between 2.5 + 2.6? Are the missing classes generated by the failing module or are they part of another module?

@cdietrich
Copy link
Contributor

the are part of the module. need to run locally. will also try to provide a more mininal example

@cdietrich
Copy link
Contributor

the problem is bootstrapping with new java/platform versions.
these might need a new xtend version

@laeubi
Copy link
Member Author

laeubi commented Jan 1, 2022

the are part of the module. need to run locally. will also try to provide a more mininal example

👍 that would be great I'll try to take a look at it.

You could try to watch out for the following things:

  • Enable debug output
  • See if there are any difference in the build-order of projects (should not matter here though)
  • are there any executions bind to a particular phase before/after the compile
  • are you using tycho-compiler mojo directly?
  • Let classpath computation happen at an earlier phase:
<plugin>
	<groupId>org.eclipse.tycho</groupId>
	<artifactId>tycho-compiler-plugin</artifactId>
	<version>${tycho.version}</version>
	<executions>
		<execution>
			<id>verify-classpath</id>
			<phase>validate</phase>
			<goals>
				<goal>validate-classpath</goal>
			</goals>
			<configuration>
			</configuration>
		</execution>
	</executions>
</plugin>

@laeubi
Copy link
Member Author

laeubi commented Jan 1, 2022

the problem is bootstrapping with new java/platform versions. these might need a new xtend version

Maybe we can reproduce this with an older version of xtext as well? And if I know the root cause it might even be possible to derive an example without xtext....

@cdietrich
Copy link
Contributor

minimal example can be found here
https://github.com/cdietrich/tycho-delay-classpath-resolution-problem

@cdietrich
Copy link
Contributor

it looks looks like the xtend maven plugin does not do its thing if the tycho compiler does not provide cp early

@laeubi
Copy link
Member Author

laeubi commented Jan 1, 2022

minimal example can be found here https://github.com/cdietrich/tycho-delay-classpath-resolution-problem

Could you add (EPL2.0?) license information and contribute this as an integration test to tycho?

You could simply add your project to https://github.com/eclipse/tycho/tree/master/tycho-its/projects and add a Testcase like this:

@Test
public void test() throws Exception {
    Verifier verifier = getVerifier("your-project-folder-name", false);
    verifier.executeGoals(asList("clean", "compile"));
    verifier.verifyErrorFreeLog();
}

@cdietrich
Copy link
Contributor

do you run tests with java 17? if yes it wont work.

@laeubi
Copy link
Member Author

laeubi commented Jan 1, 2022

it looks looks like the xtend maven plugin does not do its thing if the tycho compiler does not provide cp early

Does it work with:

<plugin>
	<groupId>org.eclipse.tycho</groupId>
	<artifactId>tycho-compiler-plugin</artifactId>
	<version>${tycho.version}</version>
	<executions>
		<execution>
			<id>verify-classpath</id>
			<phase>validate</phase>
			<goals>
				<goal>validate-classpath</goal>
			</goals>
			<configuration>
			</configuration>
		</execution>
	</executions>
</plugin>

@laeubi
Copy link
Member Author

laeubi commented Jan 1, 2022

do you run tests with java 17? if yes it wont work.

We don't have (yet) j17 for test but we already include j8+j11 in the build, so maybe this could be enhanced to run this particular test with j17.

@cdietrich
Copy link
Contributor

no xtend 2.25 does not work with java 17. this is why i hesitate to introduce this test as it would introduce a circular dependency.
calling validate-classpath indeed seems to solve the problem

@laeubi
Copy link
Member Author

laeubi commented Jan 1, 2022

it looks looks like the xtend maven plugin does not do its thing if the tycho compiler does not provide cp early

Juts forget: can you describe what the xtend maven plugin expects here? Can it be enhanced to understand delayed class-path computation? Does it use Tycho-API to do it work?

@cdietrich
Copy link
Contributor

@laeubi
Copy link
Member Author

laeubi commented Jan 1, 2022

no xtend 2.25 does not work with java 17.

Okay then I misunderstood your comment, currently all test are run with java 11

@cdietrich
Copy link
Contributor

yes but if you run them with 17 too then test will break

@laeubi
Copy link
Member Author

laeubi commented Jan 1, 2022

yes but if you run them with 17 too then test will break

As noted above this won't be a problem as we currently already support j8 + j11 JVM in the test, should we upgrade to J17 we most probably should support j8 + j11 + j17.

@cdietrich
Copy link
Contributor

we = tycho is not the same as we = xtend. so you would have to delete the test then again

@laeubi
Copy link
Member Author

laeubi commented Jan 1, 2022

calling validate-classpath indeed seems to solve the problem

I have added a release note do you think it is sufficient? If not feel free to propose a change 👍 .

we = tycho is not the same as we = xtend. so you would have to delete the test then again

given we include the test as now with xtend 2.25 should it not work with j11 'forever'? Anyways I don't want to persuade you to submit this as a test-case I just think it would be usefull and it comes with no obligations from your side, I just can't include it without creating an CQ/IP review if you do not submit it yourself.

@cdietrich
Copy link
Contributor

yes and no. sometimes we strugge with the maven version ranges in emf and platform and the non semantic versioning regading java versions there (newer stuff will be pulled that might need java 17)
will try to do a pr. but i have no idea how to test it.

@laeubi
Copy link
Member Author

laeubi commented Jan 1, 2022

i dont know the details but i assue it somehow consumes claspaths (using maven api)

I have take a look at the source it boils down to (maven 3.8.4):

public List<String> getCompileClasspathElements()
       throws DependencyResolutionRequiredException
   {
       List<String> list = new ArrayList<>( getArtifacts().size() + 1 );

       String d = getBuild().getOutputDirectory();
       if ( d != null )
       {
           list.add( d );
       }

       for ( Artifact a : getArtifacts() )
       {
           if ( a.getArtifactHandler().isAddedToClasspath() )
           {
               // TODO let the scope handler deal with this
               if ( Artifact.SCOPE_COMPILE.equals( a.getScope() ) || Artifact.SCOPE_PROVIDED.equals( a.getScope() )
                   || Artifact.SCOPE_SYSTEM.equals( a.getScope() ) )
               {
                   addArtifactPath( a, list );
               }
           }
       }

       return list;
   }

The interesting part is the call to getArtifacts() that states:

Contents are lazily populated, so depending on what phases have run dependencies in some scopes won't be included. eg. if only compile phase has run, dependencies with scope test won't be included.

so it seems you should bind the xtend-maven-plugin probably to the process-classes phase to make sure everything from compile is done?

Anyways can you put a breakpoint there and see in what the returned list differs?

@cdietrich
Copy link
Contributor

But we need to run before tycho compile for circular Java xtend calls

@cdietrich
Copy link
Contributor

So maybe Problem is also src path and not classpath

@laeubi
Copy link
Member Author

laeubi commented Jan 1, 2022

But we need to run before tycho compile for circular Java xtend calls

I just wanted to point out that currently the plugin relies on undefined behavior :-)
Is extend expected to run only together with tycho? Then you might extends org.eclipse.tycho.compiler.AbstractOsgiCompilerMojo and call getClassPath(project) there...

@cdietrich
Copy link
Contributor

Our plugin also runs in non tycho contexts

@laeubi
Copy link
Member Author

laeubi commented Jan 1, 2022

What I could think about is that we add the validate-classpath to the default life-cycle of phase initialize that should fix the issue for xtext and still support the use-cases I have in mind...

@cdietrich
Copy link
Contributor

looks good with new snapshot

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

No branches or pull requests

2 participants