-
Notifications
You must be signed in to change notification settings - Fork 147
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
Upgrade CS #771
Closed
Closed
Upgrade CS #771
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Oops, something went wrong.
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.
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.
This isn't possible.
Sevntu-checks may be fine, but eclipsecs-sevntu-plugin won't work because eclipse-cs hasn't been upgraded yet.
Sevntu is currently entirely dependent on eclipse-cs version.
New property name is very misleading in that it makes you think it is only a checkstyle dependency and not an eclipse-cs one.
I don't see how this can be merged unless we split apart version numbering between sevntu and eclipsecs-sevntu-plugin.
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 still can get the meaning of
checkstyle.eclipse-cs.version
then.How it is connected with eclipse-cs, given
sevntu.checkstyle/.ci/travis.sh
Lines 15 to 18 in 9765a3b
and
sevntu.checkstyle/eclipsecs-sevntu-plugin/pom.xml
Lines 27 to 31 in 9765a3b
Is that rather CS - eclipse-cs - sevntu compatibility level or something?
How does
checkstyle.eclipse-cs.version
influence dependency to eclipse-cs?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.
Sevntu is a library, just like checkstyle. Sevntu requires the checkstyle dependency.
eclipsecs-sevntu-plugin is a plugin which has sevntu as a dependency.
https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/eclipsecs-sevntu-plugin/pom.xml#L34-L36
It also has a dependency on the main eclipse-cs project which requires it's own checkstyle version.
https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/eclipsecs-sevntu-plugin/pom.xml#L38-L47
This is the great circle of life.
So eclipsecs-sevntu-plugin for version X requires sevntu version X which then relies on checkstyle version Y through sevntu and checkstyle version Y through eclipse-cs.
If you upgrade checkstyle version to Z, sevntu pulls that in and may work fine by itself. The problem is then eclipsecs-sevntu-plugin pulls in that same sevntu version and tries to work with that version of checkstyle while also working with eclipse-cs version of checkstyle which we know isn't version Z.
Since 2 versions aren't compatible by the updates you are required to make in this PR, something will break if we move sevntu's checkstyle version to be later than what eclipse-cs has released.
The only way to really fix this issue is either have eclipse-cs update to newer versions or break our current versioning system and allow eclipsecs-sevntu-plugin to stay stale until eclipse-cs is updated and allow new releases of sevntu alone.
I hope that helps explain the problem better.
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.
sevntu.checkstyle/.ci/travis.sh
Lines 15 to 18 in 9765a3b
Since sevntu is on checkstyle 8.18 , it looks like this SH file is outdated by displaying 8.12 unless I am missing something.
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.
Thank you for your rich response.
[As I do not use that plugin...] I'd second such solution! 😁
Indeed it does, thank you.
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.
@romani ping
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.
Yesterday I was enthusiastic about that idea as it would solve my problem. But now that I put some boxes and arrows on paper I really like that. (Not for plugin to be stale, but to separate them.) And perhaps sonar plugin as well...
Oh, I just found #705, I'll get familiar with that. It has no labels nor milestones set though. I assume you are processing it in background anyway.
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 put an approved label on it. We don't put on milestones until it is completed.