-
Notifications
You must be signed in to change notification settings - Fork 112
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
Move the tycho-version into the maven.config and unify build arguments #848
Conversation
Oh and it looks like M2E does not consider the I think this is #274 and should not be too hard now? |
c32696a
to
18f8838
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's actually an improvement. Many settings in the script are fine for CI but undesired for a local build (because the test failure/error needs to be visible on a local build).
That's right, but therefore I replaced the ignore.failure/error properties by '--fail-at-end' which seems suitable in both cases. |
|
62c1cce
to
ec62637
Compare
44e2f05
to
2e7ce84
Compare
2e7ce84
to
d52b667
Compare
d52b667
to
cd18f62
Compare
FYI I have adjusted the build here: so maybe the both approaches should be merged. |
cd18f62
to
8ddfab5
Compare
Merged your changes to and move the specification of the maven.test.error/failure.ignore properties to the CI scripts so that local builds can fail. |
@laeubi I lost a bit track of your work in regards to support properties in the maven.config and are therefore not sure if it is supposed to work now? When I prepared the update to this PR m2e did not complain in my m2e-dev workspace so I assume this can be merged as soon as we agree upon it? |
Status is it should work in all relevant cases but not sure about leminx and maybe there are still some areas uncovered :-D |
66c24b4
to
0cf7b90
Compare
OK, I can get my workspace into a state where at least m2e shows no errors about a missing @mickaelistria could you help add support for maven.configs in the language server? Lines 57 to 59 in 69a6425
|
0cf7b90
to
6393538
Compare
6393538
to
e36fccd
Compare
e36fccd
to
40140c5
Compare
2e8276a
to
354de99
Compare
Move specification of the -Dmaven.test.error/failure.ignore=true properties to the build files again, so that local builds can fail.
354de99
to
2351c6d
Compare
Requested changes were applied.
Now that we have #1692, we can finally use this. In m2e internally it seems to work, at least the effective pom shows the updated version. |
You need to edit the file to get the changes reflected I think, or the LS must watch the file for changes but as one no really change this very often I think it is acceptable and the same will happen if you for example change a parent pom file that is currently not opened as a file in the editor. @mickaelistria is there a way m2e can tell the language server to flush its cache? |
No, and as mentioned in previous discussion, the goal is that m2e decides smartly and automatically when to flush its cache by monitoring all the files that participate to resolution (including maven.config) and flush the cache on changes, like it already does for pom.xml files; without involving the client. |
I assume you mean lemminx? Where is this "automatic watching" implemented in lemminx? The main point is if two components watching files this double work can maybe avoided. |
Indeed, I mean lemminx*=maven*. There is no automatic watching implemeted in lemminx-maven at the moment, because LSP sends notifications for changed files that the clients configures as supported by this LS (currently only pom.xml files).
lemminx-maven work at best on its own without m2e. If this means duplicated work from m2e to lemminx-maven, then it's worth it. |
If there is a way to share things between m2e and the LSP I would consider it an advantage if this means less CPU-cycles are burned on a users computer and less memory is used. And having duplicated implementations is something I personally consider relatively bad and would only choose as last or at least later resort. I understand that the LemMinX-LSP want's to be standalone, but I still see an advantage in the things said before. Probably a good compromise has to be found. Anyways I think it would be best to discuss enhancements of the LSP integration in a dedicated ticket. Now that #1692 is integrated and tested, we can finally eat our own dog-food and use this in m2e after only 146 builds in Jenkins. |
This avoids duplicated specification of build parameters in the different builds.
However I'm a bit concerned because now if one runs the build locally test failures and errors do not let the build fail and one has to look it up on the console.
Alternatively we could use --fail-at-end, but then only projects are build that don't depend on the failed once. On the other hand, a test project should not have dependents? So probably that would be the better option.