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

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

Closed
pcdavid opened this issue Nov 22, 2022 · 14 comments
Closed
Milestone

Comments

@pcdavid
Copy link
Contributor

pcdavid commented Nov 22, 2022

I'd like to be able to override the location of a <repository> element in my Target Platform without resorting to text manipulations.

Let's say I have this .target with:

<location includeMode="planner" includeAllPlatforms="false" includeSource="true" includeConfigurePhase="false" type="InstallableUnit">
  <unit id="org.eclipse.gef.feature.group" version="0.0.0"/>
  <unit id="org.eclipse.gef.sdk.feature.group" version="0.0.0"/>
  <repository id="GEF-Classic" location="https://download.eclipse.org/tools/gef/classic/releases/3.15.0M1"/>
</location>

and GEF Classic has published a new nightly build that I'd like to test against.

I'd like to be able to do something like this (from inside the GMF sources):

mvn clean verify -Dtycho.location.GEF-Classic=https://download.eclipse.org/tools/gef/classic/nightly/the-nighty-to-test-against

and get Tycho to transparently use the specified location instead of what's written in the .target.

I know it's doable with some text rewriting prior to launching Tycho, so it's more of a nice-to-have.

The overall idea is to facilitate the kind of scenarios described in eclipse-gef/gef-classic#107 where an upstream project (here GEF Classic) would like to validate changes on their side by building/testing a downstream project (here GMF) against the result of their own PR.

Ideally, as part of their build they could clone GMF and run a complete GMF build (with tests) against the result of their build.

@laeubi
Copy link
Member

laeubi commented Nov 22, 2022

If you just want to override a location, you can define a mirror in your settings.xml for that location.

For real variable support there is a feature-request to enhance the target-file-format:

@vaclavHala
Copy link

Hello, I have similar need, I'd just like to use environment variable rather than system variable

<location type="InstallableUnit" ...>
    <repository location="${env_var:MY_MIRROR}/eclipse_official/releases/2020-09"/>

Currently this does not work because TargetDefinitionFile tries to parse the string to URI directly. What we'd need is to pass the location string through TargetDefinitionResolver same as it is done e.g. for paths.

I had a quick look at the sources and it seems to me this particular case (i.e. ability to use variables in repository location) could be handled by changing TargetDefinition$Repostiory#getLocation from returning URI to returning String (which could contain the ${} var) and then having TargetDefinitionResolver resolve that to URI after expanding variables.

Would you take a PR which does this? It solves way less general problem than the proposal eclipse-pde/eclipse.pde#204 you linked above, but I think it would be a useful addition for now before that one gets implemented

@laeubi
Copy link
Member

laeubi commented May 17, 2023

Would you take a PR which does this? It solves way less general problem than the proposal eclipse-pde/eclipse.pde#204 you linked above, but I think it would be a useful addition for now before that one gets implemented

As mentioned first PDE needs to support this because otherwise the target file would be invalid.

Also your example seem to indicate you wanted to actually use a different (mirror) location, therefore you can simply define a mirror in your settings.xml and use any maven supported variable replacement there.

@vaclavHala
Copy link

vaclavHala commented May 17, 2023

With the env_var eclipse seems to process the target file (almost) correctly. Content of the locations gets resolved fine, just their title in the UI is messed up

Screenshot_2023-05-17_13-27-25

I'm running

Eclipse IDE for RCP and RAP Developers (includes Incubating components)

Version: 2022-03 (4.23.0)
Build id: 20220310-1457

Problem with the maven mirrors replacement is the settings.xml file does not get picked up by eclipse and I'd like to be able to pick any location in there also, not only when building via mvn.

Another downside of the mirror-via-settings.xml is we have around 15 different repos we wish to mirror and occasionally need to add more. With the templating approach I just add the new location into the target and given I use the correct env_var in its location it will be directed at the correct URL. With the settings.xml I need to add full new URL with matching ID into the settings.xml and everyone needs to update their environment (since settings.xml is not in git) to get the new repo mirrored correctly. If it were at least possible to pass multiple settings files into mvn (so I could have one that contains just the mirrors and can be shared and then everyone has their own local settings and these get merged at runtime) this would be less of a problem, unfortunately I'm not familiar with such mechanism

@laeubi
Copy link
Member

laeubi commented May 17, 2023

With the env_var eclipse seems to process the target file (almost) correctly. Content of the locations gets resolved fine, just their title in the UI is messed up

Okay, then we can support this as well, would be good to report the issue with the title to PDE, but you should
provide an integration-test for this new feature to make sure it will working in future Tycho releases as well.

@vaclavHala
Copy link

Here is the PR #2421
and here is issue in PDE for the location titles in GUI eclipse-pde/eclipse.pde#599

@laeubi
Copy link
Member

laeubi commented May 18, 2023

I jsut see you are targeting 2.7.x branch, the best is to start at master and if that succeeds head over to probably any older branches.

@vaclavHala
Copy link

I almost have PR for master branch ready, last thing I have trouble with is how to inject into Mojos.
Basically I need something like this

@Mojo(name = "update-target")
public class UpdateTargetMojo extends AbstractUpdateMojo {

    @Requirement
    private TargetDefinitionResolverService definitionResolver;

in 2.7.x I did injecting by this.equinox.getService(); but this field is no longer available in master branch.
I tried using both @Requirement and @Component plexus annotations on the field but neither seems to have any effect. I admit I'm not very intimately familiar with the plexus world and just trying things at random seems to lead nowhere

Any hint how to do this please @laeubi ?

@laeubi
Copy link
Member

laeubi commented May 22, 2023

For Mojo use @Component, but update-target seems not the right place to archive this, variables are resolved in the org.eclipse.tycho.p2resolver.TargetDefinitionResolver.resolveContentWithExceptions(TargetDefinition, IProvisioningAgent) method (just search for usages of resolvePath)

@vaclavHala
Copy link

well there are three places I found where I need to update the URI handling

  • resolution for main invocation in TargetPlatformFactoryImpl.resolveTargetDefinitions
  • UpdateTargetMojo which does TargetDefinitionFile.parse then passes that into p2.getTargetPlatformAsResolutionResult which reads repo URIs from it
  • TPValidationMojo which does TargetDefinitionFile.read and then tries to use repo URIs from it directly

In all three cases I pass the raw uri String throught TargetDefinitionResolverService which delegates to TargetDefinitionResolver (which has complicated ctor and can't be injected).
I did not want to put the resolution logic into TargetDefinitionFile because that seems to deal with actual content of the file as read from disk and has static methods without invocation context.
So I put the resolution logic into TargetDefinitionResolver but now need to funnel the URIs into it from all places in code which read TargetDefinitionFile (which includes the two Mojos into which I'd like to somehow inject the TargetDefinitionResolverService service)

@laeubi
Copy link
Member

laeubi commented May 22, 2023

At least from the initial report you want to have variable in target files (even though the sound similar that are different things to target platform), so any variable support must go into the TargetDefinitionResolver.resolveContentWithExceptions(TargetDefinition, IProvisioningAgent) what maps a location to something that can actually be downloaded/used and where URIs are resolved, a target file itself has no context at all, thats why we can only resolve certain things (like ENV and system properties) that are global to the reactor, these are never mojo specific, UpdateTargetMojo is mostly outdated and the same is true for TPValidationMojo both should probably better be deleted anyways so you can leave those alone.

@vaclavHala
Copy link

Here is PR into master #2425

For now I kept the modifications to Mojos I did in the PR but if you say so I can remove them.
We never used the UpdateTargetMojo but we actually do use TPValidationMojo in our build which will not work without doing the variable resolution (there are ITs that show this in the PR).

When you say the validation mojo is outdated does that mean it would be safe for us to remove this from our build as its function has somehow been moved into other parts of tycho? I think we originally added the validation into our build as it caught some errors that otherwise bubbled up during the build much later after the initial resolution phase

@laeubi
Copy link
Member

laeubi commented May 22, 2023

I think we originally added the validation into our build as it caught some errors that otherwise bubbled up during the build much later after the initial resolution phase

If you don't know why you use it, you should remove it :-)
In general no one seems to really using this mojo, it is limited to P2 only and it increases the buildtime as it bypass usual Tycho target handling.

vaclavHala pushed a commit to vaclavHala/tycho that referenced this issue May 23, 2023
vaclavHala pushed a commit to vaclavHala/tycho that referenced this issue May 26, 2023
vaclavHala pushed a commit to vaclavHala/tycho that referenced this issue May 26, 2023
vaclavHala pushed a commit to vaclavHala/tycho that referenced this issue May 29, 2023
vaclavHala pushed a commit to vaclavHala/tycho that referenced this issue May 29, 2023
laeubi pushed a commit that referenced this issue May 30, 2023
@laeubi
Copy link
Member

laeubi commented May 30, 2023

This is now possible and will soon appear in the snapshots, thanks @vaclavHala for taking care here.

@laeubi laeubi closed this as completed May 30, 2023
@laeubi laeubi added this to the 4.0 milestone Jun 24, 2023
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

No branches or pull requests

3 participants