PR for issue 1711 - master #2425

vaclavHala · 2023-05-22T11:46:28Z

No description provided.

github-actions · 2023-05-22T13:34:27Z

Test Results

  552 files +3   552 suites +3 4h 29m 35s ⏱️ - 10m 6s
  349 tests +2   343 ✔️ +3   6 💤 ±0 0 ❌ ±0
1 047 runs +6 1 028 ✔️ +7 19 💤 ±0 0 ❌ ±0

Results for commit 87068d3. ± Comparison against base commit 075f12f.

♻️ This comment has been updated with latest results.

laeubi

This looks fine in general but some adjustments should be performed!

tycho-core/src/test/java/org/eclipse/tycho/p2resolver/TargetDefinitionFileTest.java

laeubi · 2023-05-23T04:35:01Z

tycho-core/src/main/java/org/eclipse/tycho/p2resolver/TargetDefinitionResolverService.java

@@ -117,6 +118,11 @@ private void debugCacheMiss(ResolutionArguments arguments) {
        }
    }

+    public URI resolveRepositoryLocation(String location) {


Can we probably move the code for resolving variables from TargetDefinitionResolver into TargetDefinitionResolverService then? Also I think there should be exactly one method to replace variables, the parsing into an URL should happen at the relevant place, or we can have helper methods that call the generic one like resolveAsURI, resolveAsFile resolveAs...

Can we probably move the code for resolving variables from TargetDefinitionResolver into TargetDefinitionResolverService then?

Well yes, but then we have to pass TargetDefinitionResolverService into TargetDefinitionResolver as some sort of callback for resolving variables because the resolve* methods are needed deep in the code of TargetDefinitionResolver.
Maybe you had something like this in mind?

interface TargetDefinitionVariableResolver { String resolveVariable(String raw); } class TargetDefinitionResolverService implements TargetDefinitionVariableResolver{ String resolveVariable(String raw){ // move code for env_var, systep_prop and project_loc here } CompletableFuture<TargetDefinitionContent> resolveFromArguments(ResolutionArguments arguments) { ... var resolver = new TargetDefinitionResolver(this, arguments.environments, ...); resolver.resolveContent(...) } class TargetDefinitionResolver { final TargetDefinitionVariableResolver varResolver; TargetDefinitionResolver(TargetDefinitionVariableResolver varResolver, ...){} TargetDefinitionContent resolveContentWithExceptions(TargetDefinition definition, ...){ ... } else if (locationDefinition instanceof PathLocation pathLocation) { String resolvePath = varResolver.resolvePath(pathLocation.getPath(), definition); ... } }

personally I prefer having the resolution in TargetDefinitionResolver and delegating to it from TargetDefinitionResolverService where necessary but it is your call

Also I think there should be exactly one method to replace variables,

ok I can do that, I did it as a separate method because I didn't want repository location to support project_loc as that makes no sense to me but I guess there is no harm in having that option available and just never user rather than creating special value for it

Having a TargetDefinitionVariableResolver makes sense, maybe even as a separate component (it seems the mojos only reference TargetDefinitionResolverService because the actually want to resolve variables).

I did it as a separate method because I didn't want repository location to support project_loc as that makes no sense to me but I guess there is no harm in having that option available and just never user rather than creating special value for it

Yes we should just support "variables" the user has to decide what is the best one to use...

What is the plexus way of doing @PostConstruct?

I created the TargetDefinitionVariableResolver but now I need to get the logger from mavenContext after injecting it

@Component(role = TargetDefinitionVariableResolver.class) public class TargetDefinitionVariableResolver { @Requirement private MavenContext mavenContext; @Requirement // this does not work private MavenLogger logger; @PostConstruct // this does not work either private void postConstruct(){ // I expect the DI framework to call this method after it injected all members this.logger = mavenContext.getLogger(); }

of course I could get the logger at the start of the resolve method (so it sets the logger the first time it is called and does effectively noop on each subsequent call) but that seems ugly

please don't use MavenLogger (that legacy) but simply plexus logger you can inject directly in your component.
Also if not required by method definition MavenContext should not be used, both things are from the old days where Tycho has slit into a maven and a mebedded osgi part.

beside that Plexus has "Initlizable" interface for post construct actions.

well both org.eclipse.tycho.core.shared.MavenContext and org.eclipse.tycho.core.shared.MavenLogger were used in the original TargetDefinitionResolver, I just moved the code from it into TargetDefinitionVariableResolver without doing any further changes.

If you wish I can replace the logger there by injected org.codehaus.plexus.logging.Logger.
As for the mavenContext it was used for
mavenContext.getSessionProperties() and mavenContext.getProjects() - what should I replace these by to avoid the MavenContext?

thanks for the post construct hint

laeubi · 2023-05-23T04:36:03Z

...m-validation-plugin/src/main/java/org/eclipse/tycho/extras/tpvalidator/TPValidationMojo.java

@@ -194,8 +198,9 @@ private void validateTarget(File targetFile) throws TPError {
            for (Location location : targetDefinition.getLocations()) {
                if (location instanceof InstallableUnitLocation p2Loc) {
                    for (Repository repo : p2Loc.getRepositories()) {
-                        ref.addArtifactRepository(repo.getLocation());
-                        ref.addMetadataRepository(repo.getLocation());
+                        var repoUri = definitionResolver.resolveRepositoryLocation(repo.getLocation());


please don't use var for trivial types.

laeubi · 2023-05-23T04:39:12Z

tycho-its/src/test/java/org/eclipse/tycho/test/target/TargetVariableResolutionTest.java

+
+	@Test
+	public void repositoryUrlCanContainEnvVarVariable() throws Exception {
+		Verifier verifier = getVerifier("target.variables-env", false);


can we use subfolders instead of dash to separate different use cases e.g.

Suggested change

Verifier verifier = getVerifier("target.variables-env", false);

Verifier verifier = getVerifier("target.variables/env", false);

just in case more tests are added this is easier to manage.

laeubi · 2023-05-23T04:39:27Z

tycho-its/src/test/java/org/eclipse/tycho/test/target/TargetVariableResolutionTest.java

+
+	@Test
+	public void repositoryUrlCanContainSystemPropertyVariable() throws Exception {
+		Verifier verifier = getVerifier("target.variables-sysprop", false);


Suggested change

Verifier verifier = getVerifier("target.variables-sysprop", false);

Verifier verifier = getVerifier("target.variables/sysprop", false);

laeubi · 2023-05-23T04:40:53Z

tycho-targetplatform/src/main/java/org/eclipse/tycho/targetplatform/TargetDefinitionFile.java

@@ -563,6 +563,10 @@ public static boolean isTargetFile(File file) {
 	            && !file.getName().startsWith(".polyglot.");
 	}

+	public static TargetDefinition.Repository repository(String id, String uri) {


Why is this required and one can not just call the constructor directly?

I found this less intrusive than making the private type public, but if you have no problem with that I'll just increase visibility on the Repository class and remove this

But where is this method actually used?

in UpdateTargetMojo.LatestVersionLocation#getRepositories when creating copy of repository with its location resolved

In this case it would be better to have a private implementation in UpdateTargetMojo that can delegate everything to original item but return a different URL ...

laeubi · 2023-05-23T04:46:07Z

@vaclavHala also please add an entry to the release notes so people are aware of that feature and link the Issue in your commit message.

vaclavHala · 2023-05-24T12:26:27Z

@laeubi I pushed new commits addressing all your comments and resolved conflicts with the latest master.
Please when you find a moment have a look and let me know if you are happy with the PR now or I should do some more changes

laeubi

Thanks for the adjustments I think it looks quite good now just some smaller adjustments/hints here.

tycho-core/src/main/java/org/eclipse/tycho/p2resolver/TargetDefinitionVariableResolver.java

laeubi · 2023-05-24T13:46:34Z

tycho-core/src/main/java/org/eclipse/tycho/p2resolver/TargetDefinitionVariableResolver.java

+
+    @Requirement
+    private MavenContext mavenContext;
+    private MavenLogger logger;


Please use org.codehaus.plexus.logging.Logger like this

Suggested change

private MavenLogger logger;

@Requirement

private Logger logger;

that way we don't need the Initializable nor the MavenContext

I replaced the logger as you suggest, but we still do need the MavenContext. It was not used just to get to the logger, I also need it to mavenContext.getProjects() and mavenContext.getSessionProperties(). What are suitable replacements for these?

You can take a look at DefaultMavenContext class,

mavenContext.getSessionProperties() --> Session.getProperties()
mavenContext.getProjects() --> Session.getProjects()

but if you like you can just retain this for now and only replace the logger, its just good to remove references to the MavenContext where it is easily possible (like loggers).

I looked there but if I understand you correctly that would mean instead of MavenContext I'd have to inject LegacySupport and copy/paste the get(Session|Propeties|Projects) methods from DefaultMavenContext into the TargetDefinitionVariableResolver which hardly seems as an improvement

If you want access to the session, you can add @SessionScoped to the component and add a constructor like this:

@Inject public MySessionScopedComponent(MavenSession mavenSession) { this.mavenSession = mavenSession; }

You should never need to use any mocks to test your changes, only test the user visible API and/or using real integration test projects.

Beside that, if you want to change the session values in a test you can override org.eclipse.tycho.testing.TychoPlexusTestCase.modifySession(MavenSession)

Regarding failures like:

Caused by: java.lang.NullPointerException: Cannot invoke "java.io.File.getName()" because the return value of "org.eclipse.tycho.ReactorProject.getBasedir()" is null

one should simply add a null-check at that location.

one should simply add a null-check at that location.

I don't think I agree here, these tests worked fine before but started breaking when I changed MavenContext in the variable resolver for MavenSession to look up current projects. The MavenSession that gets injected provides different list of projects than MavenContext. Projects provided by the MavenSession (as shown in my comment above) do not have the basedir set which causes these NPE failures.

I agree mocking is probably not the way to do here. However simply injecting the MavenSession leads to the test failures for reasons I do not currently understand and don't think I could address in reasonable time. So since using MavenSession instead of MavenContext is not a matter of simple 1:1 replacement I suggest we do not go through with it in this PR and keep the MavenContext for now

So since using MavenSession instead of MavenContext is not a matter of simple 1:1 replacement I suggest we do not go through with it in this PR and keep the MavenContext for now

That would be okay.

laeubi · 2023-05-24T13:49:10Z

.../tycho-version-bump-plugin/src/main/java/org/eclipse/tycho/versionbump/UpdateTargetMojo.java

@@ -178,4 +192,26 @@ public String getVersion() {

    }

+    private static final class Repository implements TargetDefinition.Repository {


You might want to choose a more descriptive name e.g.

Suggested change

private static final class Repository implements TargetDefinition.Repository {

private static final class ResolvedRepository implements TargetDefinition.Repository {

vaclavHala · 2023-05-29T07:09:06Z

@laeubi I pushed some new commits, I believe all your comments are now addressed. Please let me know if you need me to do anything more before this can be merged.

Also, what needs to be done for this new feature to be backported into 2.7.x so it is included in 2.7.5 release? I can update #2421 to make it match changes in this PR as closely as possible

laeubi

Looks good, some header adjustments for completeness required then its fine to merge.

laeubi · 2023-05-29T08:46:10Z

tycho-core/src/main/java/org/eclipse/tycho/p2resolver/TargetDefinitionVariableResolver.java

@@ -0,0 +1,23 @@
+/*******************************************************************************
+ * Copyright (c) 2011, 2022 SAP SE and others.


Suggested change

* Copyright (c) 2011, 2022 SAP SE and others.

* Copyright (c) 2023 Vaclav Hala and others.

laeubi · 2023-05-29T08:47:36Z

...core/src/main/java/org/eclipse/tycho/p2resolver/DefaultTargetDefinitionVariableResolver.java

+ *
+ * SPDX-License-Identifier: EPL-2.0
+ * Contributors:
+ *    Vaclav Hala - initial API and implementation


Suggested change

* Vaclav Hala - initial API and implementation

* Vaclav Hala - extraction code from TargetDefinitionResolver into dedicated component

laeubi · 2023-05-29T08:50:06Z

tycho-its/src/test/java/org/eclipse/tycho/test/target/TargetVariableResolutionTest.java

@@ -0,0 +1,61 @@
+/*******************************************************************************
+ * Copyright (c) 2011, 2022 SAP SE and others.


Suggested change

* Copyright (c) 2011, 2022 SAP SE and others.

* Copyright (c) 2023 Vaclav Hala and others.

laeubi · 2023-05-29T09:32:46Z

@vaclavHala also please rebase/squash all changes into one commit if possible.

Also, what needs to be done for this new feature to be backported into 2.7.x so it is included in 2.7.5 release? I can update #2421 to make it match changes in this PR as closely as possible

If code has diverged to much its most often easier to "start from scratch" and just try to insert the interesting parts accordingly also its most often good to not perform refactoring in backports, even though this might lead to duplication or not so clean code, but for a backport smaller local changes are usually better.

included in 2.7.5 release

I fear a new release of the 2.7.5 is quite unlikely if we can get the CI working (again) there should be a SNAPSHOT to test at laest, but you might want to backport it to 3.x first there will be a release after the 2023-06 eclipse release (about two week from now on).

…ar and system_property variables

vaclavHala · 2023-05-29T10:18:16Z

I pushed new commit with the header changes you wanted and everything squashed.

included in 2.7.5 release

I meant next release in the 2.7.x line, I see I mistyped the exact version as 2.7.5 already exists, so 2.7.6.
We'd like to see this in the 2.7.x as we are not yet on java 17 and I'm not sure how feasible upgrading to that would be at the moment for us. So that said 3.x does not help us as that also requires java 17.

For now please let's just merge this into master and we will discuss internally how we want to proceed with the backporting/upgrading first

laeubi · 2023-05-29T13:33:42Z

This looks good now and is ready to be merged.

We'd like to see this in the 2.7.x as we are not yet on java 17 and I'm not sure how feasible upgrading to that would be at the moment for us.

Please note that you only need java 17 to run the maven build, the code can still be all java 11 (or 1.8 or ..) so if you see any problems regarding this usecase let us know!

vaclavHala mentioned this pull request May 22, 2023

Add support for overriding a Target Platform location from the command-line #1711

Closed

laeubi reviewed May 23, 2023

View reviewed changes

laeubi reviewed May 24, 2023

View reviewed changes

vaclavHala force-pushed the issue_1711_reproducer_master branch from 8fcb6a8 to 80d4db0 Compare May 26, 2023 10:00

laeubi approved these changes May 29, 2023

View reviewed changes

issue eclipse-tycho#1711 target repository location can contain env_v…

87068d3

…ar and system_property variables

vaclavHala force-pushed the issue_1711_reproducer_master branch from f01fefa to 87068d3 Compare May 29, 2023 10:09

vaclavHala force-pushed the issue_1711_reproducer_master branch from 3302ad0 to 87068d3 Compare May 29, 2023 12:36

laeubi merged commit 65d1031 into eclipse-tycho:master May 30, 2023

laeubi added this to the 4.0 milestone Jun 24, 2023

laeubi mentioned this pull request Jan 23, 2024

PR for issue 1711 #2421

Closed

	Verifier verifier = getVerifier("target.variables-env", false);
	Verifier verifier = getVerifier("target.variables/env", false);

	private MavenLogger logger;
	@Requirement
	private Logger logger;

		@@ -178,4 +192,26 @@ public String getVersion() {

		}

		private static final class Repository implements TargetDefinition.Repository {

	private static final class Repository implements TargetDefinition.Repository {
	private static final class ResolvedRepository implements TargetDefinition.Repository {

		@@ -0,0 +1,23 @@
		/*******************************************************************************
		* Copyright (c) 2011, 2022 SAP SE and others.

	* Copyright (c) 2011, 2022 SAP SE and others.
	* Copyright (c) 2023 Vaclav Hala and others.

	* Vaclav Hala - initial API and implementation
	* Vaclav Hala - extraction code from TargetDefinitionResolver into dedicated component

		@@ -0,0 +1,61 @@
		/*******************************************************************************
		* Copyright (c) 2011, 2022 SAP SE and others.

PR for issue 1711 - master #2425

PR for issue 1711 - master #2425

Conversation

vaclavHala commented May 22, 2023

github-actions bot commented May 22, 2023 • edited Loading

Test Results

laeubi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laeubi commented May 23, 2023

vaclavHala commented May 24, 2023

laeubi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaclavHala commented May 29, 2023

laeubi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laeubi commented May 29, 2023

vaclavHala commented May 29, 2023

laeubi commented May 29, 2023

github-actions bot commented May 22, 2023 •

edited

Loading