Fix #20 - XMLParser should not require a bundle context but a Parser in the constructor #21
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
laeubi
force-pushed
the
issue_20
branch
3 times, most recently
from
15b9f6d to
01e2de3
Compare
the constructor
mickaelistria
requested changes
Mar 29, 2022
| @@ -192,31 +192,31 @@ private Location getLockLocation(URI repositoryLocation) throws IOException { | |||
| // Constants defining the structure of the XML for a SimpleArtifactRepository | |||
|
|
|||
| // A format version number for simple artifact repository XML. | |||
| public static final Version COMPATIBLE_VERSION = Version.createOSGi(1, 0, 0); | |||
There was a problem hiding this comment.
Why changing that here? Is it required as part of the change?
I'd rather not change visibility here as it's very likely some other stuff (wrongly) depends on it; and it doesn't seem related to the desired improvement.
Also, static final should at least remain.
There was a problem hiding this comment.
"public static final" is the default in interfaces and thus my eclipse-sdk removes unnecessary modifiers here. It seems I have to somewhat fore ommph not reformatting/cleanup code, but maybe we simply could keep it as it is equivalent?
There was a problem hiding this comment.
My bad. Let's merge.
mickaelistria
approved these changes
Mar 29, 2022
|
It looks like this change requires some adjustments in Tycho. |
|
After I simply reverted this change locally P2 and subsequently Tycho build fine without further changes. |
|
Can you share a Stack trace that shows where the problem occurs? |
Yes. After basically removing the BundleContext argument from the constructors of |
Could it be due to the recent removed / changed providers of some packages? I have a while back already did as you said and removed the BC from constructor and it has worked, so I'm a bit surprised we are seeing issues here. Are you sure that you have only P2 changed / consumed from your local repo? I generally use |
|
Strange.
Relatively sure. I cannot remember having installed another Eclipse project into my local Maven-repo. And I deleted the org/eclipse/equinox respectively tycho folders in my local m2-repo before a p2+tycho installation round. I can only say that reverting this change on top of my change in #64 and building/installation p2+tycho again fixed the issue for me locally. But you are right. The potential for interference is great here. I will try your suggestion later this evening and can report back. |
|
At least the error does not indicate a problem in the code itself but some class-path issues, so there must have changed something else beside the usual removing of the BC that triggers this, one thing that might interfere here is that we use P2 in pom and in target (and thus maybe the pom.xml has to be adjusted as well), but I don't see yet why reverting this change should make a difference here... this reminds me of #23 and that we should get a faster feedback cycle from p2->tycho ... |
That sounds reasonable. I'll try that.
Agree. :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.