-
Notifications
You must be signed in to change notification settings - Fork 31
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
[critical] lemminx-maven doesn't take global settings file into account. #42
Comments
@mickaelistria What should we do, How we can take in the settings we have in m2e for global configuration. If we take the global configuration that will solve many issues I see. One way would be to first try to M2 environment variable and fallback to maven.repo.local system settings and fallback to hardcoded one when resolving the settings file. The best is the take the m2e settings, but I think that will be challenging this is running as a separate process. |
We currently have a system property to define the location of the maven local repository. Maybe if an environment variable was used, the user could specify the location of their local maven repository.
For mirrors defined in the global Since this project is editor agnostic (it's an extension to a language server which can be used by any LS client-editor), we can't call the m2e API to get the settings files. |
Yes the M2 environment variable should probably be used |
I started investigating about preferences in #34 and it's kind of working-ish. However, we'd need to do that eagerly and look first at settings, and then override what has to be overridden later.
|
@mickaelistria can we pass the system properties you care reading at SettingsXmlConfigurationProcessor as VM arguments at https://github.com/eclipse/wildwebdeveloper/blob/0daf7398e1fa7aa8adfda6585d74fec6a9badff2/org.eclipse.wildwebdeveloper.xml/src/org/eclipse/wildwebdeveloper/xml/internal/XMLLanguageServer.java#L41 maybe we can only pass some prefixed system properties from the parent jvm to xmlserver jvm. |
We'll see in a later patch, as we first need to have settings working anyway, at least default one before adding some customization here. |
I improved #34 so at least the default local and global settings should now be considered (eg mirrors available). |
I will try building you branch and patching m2e. |
Fixed with #34. Please close. |
On windows with custom repository mirrors, the parent pom resolution doesn't work. "Non-resolvable parent POM for testing:workbench:0.0.1-SNAPSHOT: Could not find artifact" @mickaelistria @AObuchow |
I tried the same setting file at ~/.m2/settings.xml as well, but still the problem is there for parent artifact. |
i even tried setting the m2e configuration using "/" instead of "", doesn't help. It seems like the dependencies are also only resolved from default central repo. |
I revert the #34 changes and tried with the |
Can you please elaborate how you did pass the maven.conf parameter? Did you have it running in m2e? What value did you pass? |
I passed in the path to conf directory in my maven installation. The code before the fix #34 will read the setting.xml file from that location. |
@mickaelistria @AObuchow event with the lastest lemminx-maven i still have the dependency resolution issues when we have a mirror repo in settings :( |
Can you please try to write some unit test for LemMinX covering this issue?
|
@gayanper this issue is still present, correct? @mickaelistria maybe this should be added to the 0.0.1 milestone and prioritized for the release? Perhaps I can come up with a test or fix. |
I apparently didn't merge the patch in m2e to force usage of more recent lemminx-maven, so the snapshot build uses Lemminx-Maven from the 15th (12 days ago) and misses numerous patches. |
@mickaelistria thanks for the update. Sounds good to me :) |
@gayanper m2e was updated properly. Can you please try again? |
@mickaelistria The problem is still there with 1.16.0.20200527-2124. I still get error when resolving the parent pom |
OK. |
Configuration in M2E I can't add the pom file since is proprietary project. I have attached the setting file after removing all the cooperate information. |
Can you please rework the project to a minimal reproducer that you can share? We really cannot make good progress without it. |
@gayanper Can you help there regarding my previous comment? Without more specific example of how to reproduce very soon, this will miss 2020-06. |
@gayanper now that there's Windows CI, it might be more easily doable to reproduce this on our CI. If you need help in setting up a test case, just let me know. |
I tested this with latest build after windows file path fix. And it works now. Global settings are correctly taken into account. |
@gayanper awesome!! Thank you for reporting and testing this. I'm going to close this for now. |
My mistake @AObuchow the global settings xml is not read. I had the global settings file sym linked to ~/.m2/settings.xml on windows. If I remove this symbolic link I still get the error for parent pom resolutions. |
Thanks for reporting @gayanper, reopening. |
A correction, I think the lemmix-maven needs to repository under ~/.m2/repository than the settings file. |
@gayanper I tried reproducing a simple test of setting a property in my global settings and checking for completion of that property. This feature doesn't seem to exist in the old m2e pom editor. If you're able to give some more specific details about how to reproduce, that'd help (are you setting a maven profile with a remote repository? or are you setting the in your settings.xml? Does m2e's old pom editor have the expected behavior?) Part of my settings.xml: <profiles>
<profile>
<id>inject-application-home</id>
<properties>
<application-home>/path/to/application</application-home>
</properties>
</profile>
</profiles>
<activeProfiles>
<activeProfile>inject-application-home</activeProfile>
</activeProfiles> I tried completing for |
Also, global settings.xml snippet: <pluginGroups>
<pluginGroup>org.eclipse.tycho</pluginGroup>
</pluginGroups> Test pom.xml: <project xmlns="http://maven.apache.org/POM/4.0.0"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
<modelVersion>4.0.0</modelVersion>
<groupId>org.test</groupId>
<artifactId>test</artifactId>
<version>0.0.1-SNAPSHOT</version>
<build>
<plugins>
<plugin>
<artifactId>tycho-compiler-plugin</artifactId>
<version>1.7.0</version>
</plugin>
</plugins>
</build> |
Something that could be useful for CI/testing with GitHub Actions: https://github.com/marketplace/actions/maven-setings-action & https://github.com/marketplace/actions/maven-settings-temporary (although it seems to work only for non-global settings) |
I found something regarding maven profiles that might be useful/relevant in the Maven Model Builder source: https://github.com/apache/maven/blame/master/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java#L261 |
Settings tests could potentially be written by setting a custom settings file, eg. here. Something to the effect of: File globalSettingsFile = overrideSettings ? getOverridenSettingsPath() :
getFileFromOptons(options.get("maven.globalSettings"), SettingsXmlConfigurationProcessor.DEFAULT_GLOBAL_SETTINGS_FILE);
if (globalSettingsFile.canRead()) {
mavenRequest.setGlobalSettingsFile(globalSettingsFile);
Settings globalSettings = reader.read(globalSettingsFile, null);
requestPopulator.populateFromSettings(mavenRequest, globalSettings);
} |
@gayanper Can you please try m2e 1.16.2, which contains newer version of lemminx-maven and report whether this works better for you? |
Closing as there is no feedback to my last question. Feel free to reopen and provide as much details to reproduce as possible if you can reproduce the issue with latest version. |
The current implementation depends on the default maven local repository. Also it doesn't support to read the setting files specified in the m2e settings. This cause issue with custom maven configuration where we have defined mirrors.
The text was updated successfully, but these errors were encountered: