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

Maven global settings and toolchains UI cleanup #1687

Conversation

G-Ork
Copy link
Contributor

@G-Ork G-Ork commented Feb 18, 2024

I redesigned the Maven user settings and add the sections Global & User. For booth there are now settings & toolchains options.

  • Added default path hints for global settings and global toolchains. They are only rendered for external maven installations. Don't forget to apply changes to see them.
  • Unify settings override through goals field. All four settings could be put into the goal in short form (-gt. -gs, -t, -s).
  • Throw exception during launch if path to a settings file is invalid.
  • Provide unit-tests for maven launcher for all four settings
  • Fix a bug in settings validation. Parser problems are not copied to result if parser do not crash. All well formed xml-files are treated as valid.
  • Add XSDs for settings and toolchains as eclipse catalog contributions.
    • Could not tests this as i am not able to start M2E-IDE this the WST XML feature.

- cleanup user settings dialog
- Fix settings validation
- add missing XSDs to eclipse XML catalog contribution
- provide unit test for maven launcher
Copy link

github-actions bot commented Feb 18, 2024

Test Results

  214 files  ± 0    214 suites  ±0   21m 36s ⏱️ + 3m 35s
  676 tests +12    665 ✅ +11  10 💤 ±0  1 ❌ +1 
1 352 runs  +24  1 328 ✅ +22  22 💤 ±0  2 ❌ +2 

For more details on these failures, see this check.

Results for commit 8e37c42. ± Comparison against base commit 044cb60.

♻️ This comment has been updated with latest results.

…@github.com:G-Ork/m2e-core.git into Maven_global_settings_and_toolchains_UI_cleanup
Maven_global_settings_and_toolchains_UI_cleanup
@laeubi
Copy link
Member

laeubi commented Feb 18, 2024

Add XSDs for settings and toolchains as eclipse catalog contributions.

Could not tests this as i am not able to start M2E-IDE this the WST XML feature.

@mickaelistria @fbricon can you probably help here?

@laeubi
Copy link
Member

laeubi commented Feb 18, 2024

@G-Ork can you share a screenshot before/after?

@G-Ork
Copy link
Contributor Author

G-Ork commented Feb 18, 2024

@laeubi Holy moly fast. Explain more what kind of screenshot you like.

@laeubi
Copy link
Member

laeubi commented Feb 18, 2024

I redesigned the Maven user settings and add the sections Global & User. For booth there are now settings & toolchains options.

So what ever you redesigned in the UI, e.g dialog before your change, and how it looks now :-)

* @author Georg Tsakumagos
*/
@FunctionalInterface
public interface IBiConsumer<T, U> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public interface IBiConsumer<T, U> {
public interface CoreBiConsumer<T, U> {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay sounds better. Same applies to the supplier.

@laeubi
Copy link
Member

laeubi commented Feb 18, 2024

@G-Ork you also need to consider two other places, one is

org.eclipse.m2e.core.internal.embedder.MavenExecutionContext.createExecutionRequest(IMavenConfiguration, IComponentLookup, MavenSettingsLocations, File)

where we need to set the Global/User Toolchain file initally, then in org.eclipse.m2e.core.internal.embedder.MavenExecutionContext.newExecutionRequest() you probabbly need to update them if the are configured in the MavenProperties differently

@G-Ork
Copy link
Contributor Author

G-Ork commented Feb 18, 2024

Okay i was not exact. I am able to start the M2E-IDE but the WST XML feature are not loaded. Here is the screenshot based on this PR.

Current (released)
Maven-Usersettings-Released

Draft from PR
Maven-Usersettings-Draft

@G-Ork
Copy link
Contributor Author

G-Ork commented Feb 18, 2024

@G-Ork you also need to consider two other places, one is

where we need to set the Global/User Toolchain file initally, then in org.eclipse.m2e.core.internal.embedder.MavenExecutionContext.newExecutionRequest() you probabbly need to update them if the are configured in the MavenProperties differently

@laeubi From what i can see it your feature is not complete. The .mvn/maven.config can contain much more than just alternative settings location.

Any chance we can refactor this to be more easy and support more options like .e.g. "-T3 -U "?
As far i can see it should work because you only care for the settings file. This part uses the configuration pointing to the settings files.

@laeubi
Copy link
Member

laeubi commented Feb 18, 2024

From what i can see it your feature is not complete.

That's why I said you probably want to enhance it to support toolchains

Any chance we can refactor this to be more easy and support more options like .e.g. "-T3 -U "?

There is no easy way as not all options can be supported and probably need to be supported differently. For example -T does not makes any sense as m2e only ever uses one build thread internally so claiming to use more might even be confusing. -U looks easy enough at a first glance, but this is usually controlled by the UI so when should we set it?

So I think one should really focus on things one by one when there is a demand.

@HannesWell
Copy link
Contributor

@G-Ork thanks for this contribution.
Since you listed many points in your intial comment I wonder if it would be possible (without too much effort) to split this into multiple PRs? This would also make it easier to review.
Furthermore for contributions of non-Committers over 1000LOC we would actually need an separate IP-clearance (usually not denied, but extra work).

I guess that the UI refactoring and the core changes can probably be splitted, similar to the schema additions.

@G-Ork
Copy link
Contributor Author

G-Ork commented Feb 18, 2024

@HannesWell The high LOC includes the xsd files which are IP from maven. I think we could split XSD changes from this one into a separate PR. The changes for UI are so small i would not exclude them from the unit-tests and m2e layers.

@HannesWell
Copy link
Contributor

@HannesWell The high LOC includes the xsd files which are IP from maven. I think we could split XSD changes from this one into a separate PR. The changes for UI are so small i would not exclude them from the unit-tests and m2e layers.

Sounds good. I have to admit that I first have to recall for what the XSD files are actually used.

@G-Ork
Copy link
Contributor Author

G-Ork commented Feb 18, 2024

@G-Ork G-Ork closed this Feb 18, 2024
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.

3 participants