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

Upgrade CS #771

Closed
wants to merge 3 commits into from
Closed

Upgrade CS #771

wants to merge 3 commits into from

Conversation

pzygielo
Copy link
Contributor

Mandatory to understand and do:
0) Issue you are trying to fix/resolve has to have "approved" label.

  1. Put in description of Pull Request reference to issue if it exists. Example: "Issue: #XXXXXX"
  2. Commit message should adhere to the following rules:
    a) Must match one of the following patterns:\n"
    ^Issue #\d+: .$
    ^Pull #\d+: .
    $
    ^(minor|config|infra|doc|spelling): .*$
    b) It contains only one line of text
    c) Must not end with a period, space, or tab

To avoid multiple iterations of fixes and CIs failures, please read http://checkstyle.sourceforge.net/contributing.html

ATTENTION: We are not merging Pull Requests that not passing our CIs, but we help to resolve issues.

Thanks for reading, remove whole this message and type what you need.

@@ -16,8 +16,8 @@
<properties>
<project.build.sourceEncoding>iso-8859-1</project.build.sourceEncoding>
<!-- It is compile dependency to checkstyle, this version has to be the same as eclipse-cs depends on -->
<checkstyle.eclipse-cs.version>8.18</checkstyle.eclipse-cs.version>
<checkstyle.configLocation>https://raw.githubusercontent.com/checkstyle/checkstyle/checkstyle-${checkstyle.eclipse-cs.version}/config/checkstyle_checks.xml</checkstyle.configLocation>
<checkstyle.version>8.23</checkstyle.version>
Copy link
Contributor

@rnveach rnveach Jul 27, 2019

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.

Copy link
Contributor Author

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

git clone https://github.com/checkstyle/eclipse-cs.git
cd eclipse-cs/
git checkout 8.12.0
mvn -e install

and
<properties>
<eclipsecs.version>8.12.0-SNAPSHOT</eclipsecs.version>
</properties>

Is that rather CS - eclipse-cs - sevntu compatibility level or something?
How does checkstyle.eclipse-cs.version influence dependency to eclipse-cs?

Copy link
Contributor

@rnveach rnveach Jul 27, 2019

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.

Copy link
Contributor

@rnveach rnveach Jul 27, 2019

Choose a reason for hiding this comment

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

git clone https://github.com/checkstyle/eclipse-cs.git
cd eclipse-cs/
git checkout 8.12.0
mvn -e install

Since sevntu is on checkstyle 8.18 , it looks like this SH file is outdated by displaying 8.12 unless I am missing something.

Copy link
Contributor Author

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.

allow eclipsecs-sevntu-plugin to stay stale until eclipse-cs is updated and allow new releases of sevntu alone.

[As I do not use that plugin...] I'd second such solution! 😁

I hope that helps explain the problem better.

Indeed it does, thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

@romani ping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

allow eclipsecs-sevntu-plugin to stay stale until eclipse-cs is updated and allow new releases of sevntu alone.

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.

Copy link
Contributor

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.

@pzygielo
Copy link
Contributor Author

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.

Understood.
Thank you for review and big picture of dependencies.

@pzygielo pzygielo closed this Jul 27, 2019
@pzygielo pzygielo deleted the upgrade branch July 27, 2019 15:37
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.

2 participants