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

dev-python/pylint: bump to 2.2.2 #10907

Closed
wants to merge 3 commits into from
Closed

dev-python/pylint: bump to 2.2.2 #10907

wants to merge 3 commits into from

Conversation

TheJJ
Copy link
Contributor

@TheJJ TheJJ commented Jan 26, 2019

Pylint 2 for gentoo.

Tracker bug: https://bugs.gentoo.org/662406

@gentoo-bot gentoo-bot added new package The PR is adding a new package. assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. no signoff One or more commits do not indicate GCO sign-off. labels Jan 26, 2019
@TheJJ TheJJ changed the title dev-python/pylint: bump to 2.2.2 dev-python/pylint: bump to 2.2.2 [please reassign] Jan 26, 2019
@gentoo-bot gentoo-bot changed the title dev-python/pylint: bump to 2.2.2 [please reassign] dev-python/pylint: bump to 2.2.2 Jan 26, 2019
@gentoo-bot gentoo-bot added new package The PR is adding a new package. assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. and removed assigned PR successfully assigned to the package maintainer(s). new package The PR is adding a new package. no bug found No Bug/Closes found in the commits. no signoff One or more commits do not indicate GCO sign-off. labels Jan 26, 2019
@ghost
Copy link

ghost commented Jan 27, 2019

Other than the obvious QA problems, a few extra problems I see:

  1. Dependencies need a refresh (why pytest<3.3? why six on a py3-only package? I guess there are many others like this)
  2. I don't think you need to add a new virtual just for pylint. adding your gen_cond_dep directly in astroid will do the trick.
  3. Python 3.3 has long been removed from the tree
  4. A bump to EAPI 7 wouldn't hurt.

@TheJJ TheJJ changed the title dev-python/pylint: bump to 2.2.2 dev-python/pylint: bump to 2.2.2 [please reassign] Jan 27, 2019
@gentoo-bot gentoo-bot changed the title dev-python/pylint: bump to 2.2.2 [please reassign] dev-python/pylint: bump to 2.2.2 Jan 27, 2019
@gentoo-bot
Copy link

Copyright policy change

Please note that on 2018-09-15 Trustees have approved new Gentoo copyright policy. All contributions made to Gentoo need to follow this policy. If you include the Signed-off-by line in your commit message, you indicate that you have read the policy and agree to its terms. For more detailed explanation, please see the new Gentoo copyright policy explained article.

Pull Request assignment

Submitter: @TheJJ
Areas affected: ebuilds
Packages affected: dev-python/astroid, dev-python/lazy-object-proxy, dev-python/pylint, dev-python/typed-ast, dev-python/wrapt

dev-python/astroid: @gentoo/python
dev-python/lazy-object-proxy: @gentoo/python
dev-python/pylint: @gentoo/python
dev-python/typed-ast: @gentoo/python
dev-python/wrapt: @gentoo/python

Linked bugs

Bugs linked: 662406


In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of ConductCopyright policy (expl.) ● DevmanualGitHub PRsProxy-maint guide

@gentoo-bot gentoo-bot added assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. and removed assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. new package The PR is adding a new package. labels Jan 27, 2019
@ghost
Copy link

ghost commented Jan 27, 2019

It doesn't look like you properly cleaned dependencies. I now see that six is actually used in astroid for some reason, but enum34 and singledispatch? Also, some astroid dependencies you have are test dependencies. Also, why have pylint as astroid's test dependency. Is it really needed? It doesn't look like it when I look at the tox.ini file. Also, have you run the tests? astroid's test command look wrong to me. It should be pytest running the tests.

Also, is the wrapt bump necessary for pylint to work? From what I see in upstream, it doesn't look like it's the case. If not, let's remove it and treat that bump separately.

Also, don't use tox for tests

Also, in a few minutes, the CI will probably give you, again, a sea of red. That's because you're in a special situation: you have pylint and astroid, packages with many supported arches, gaining a dependency with few supported arches (dev-python/typed-ast). That can't work. When that happens, we need to drop arch keywords in reverse dependency and open a new KEYWORDREQ bug to re-keyword them.

@TheJJ
Copy link
Contributor Author

TheJJ commented Jan 27, 2019

Yea, I've now removed enum34 and singledispatch. What astroid deps are only for testing but not listed there? There's a pylint tox environment apparently, but we don't call it so pylint is not a dependency, indeed. I've run the tests, but I didn't change the invocation from the 1.x astroid, I'll check if there's a new invocation.

@ghost
Copy link

ghost commented Jan 27, 2019

It looks like I got confused in dependencies, I don't see deps that belong in test deps in astroid or pylint.

As for the test command, it changed upstream between 1.5.x and 1.6.x. It wasn't changed during the 1.6 bump, but that was probably a mistake. Maybe that the command still runs by chance, but it should be changed nevertheless.

Yes pylint is in upstream's tox.ini, but it's for a linting command. We don't need to lint upstream's code (also, it creates a circular dependency).

It doesn't look like pylint depends on six anymore, only astroid.

Also, maybe you haven't seen it, but the CI will tell you: pypy in pylint should be pypy3.

@TheJJ
Copy link
Contributor Author

TheJJ commented Jan 27, 2019

https://github.com/PyCQA/pylint/search?q=six&unscoped_q=six

pylint still uses some six stuff according to this search.

I'll probably need some support to fix the remaining QA checks :) I get the same output with repoman but I don't understand what the messages want to tell me to do.
For example:

   dev-python/pylint/pylint-2.2.2.ebuild: DEPEND: ~amd64(default/linux/amd64/17.0/x32)
[     '>=dev-python/astroid-2.0.0[python_targets_pypy(-)?,python_targets_python3_4(-)?,python_targets_python3_5(-)?,python_targets_python3_6(-)?,python_targets_python3_7(-)?,-python_single_target_pypy(-),-python_single_target_python3_4(-),-python_single_target_python3_5(-),-python_single_target_python3_6(-),-python_single_target_python3_7(-)]',

@ghost
Copy link

ghost commented Jan 27, 2019

Hum, yeah, but it looks like a test-only dep.

I've already explained the cause of the CI errors in my previous message: dropped arches (and also, the pypy thing).

What this specific message you quote there tells you is that you have a mismatch between PYTHON_COMPAT in pylint and astroid: for any given python package, all python deps must support all python implementations of that package. pylint lists pypy but astroid doesn't have pypy.

@TheJJ
Copy link
Contributor Author

TheJJ commented Jan 27, 2019

Now my RepoMan is happy, apparently.

Let's see if the CI passes. I see now what the message says ("can't find dependency with those flags") but I still can't infer which flag (or py version in this case) causes the problem just from the message, I guess that's normal though and you have to look at the other ebuilds by hand.

@TheJJ TheJJ force-pushed the pylint-v2 branch 3 times, most recently from db648c0 to fa7aedf Compare January 28, 2019 09:44
@TheJJ
Copy link
Contributor Author

TheJJ commented Jan 28, 2019

Lookin good. Any other change request? :)

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Other than these minor things, it's looking good on the surface. Will review fully soon.

dev-python/astroid/astroid-2.1.0.ebuild Outdated Show resolved Hide resolved
dev-python/astroid/astroid-2.1.0.ebuild Outdated Show resolved Hide resolved
dev-python/astroid/astroid-2.1.0.ebuild Outdated Show resolved Hide resolved
@ghost ghost self-assigned this Jan 28, 2019
@ghost
Copy link

ghost commented Jan 29, 2019

I forgot I had noticed this, but you didn't address one of my remarks:

you have pylint and astroid, packages with many supported arches, gaining a dependency with few supported arches (dev-python/typed-ast). That can't work. When that happens, we need to drop arch keywords in reverse dependency and open a new KEYWORDREQ bug to re-keyword them.

We can't add arch keywords unless we're in the proper arch team. A blanket keywording like you did can't be done. What you have to do it to drop the keywords on revdeps. Then, afterwards, a keyword request has to be filed to re-keyword revdeps (I can take care of it after I push your commits).

@TheJJ
Copy link
Contributor Author

TheJJ commented Jan 30, 2019

Ah, I didn't see your remark because it was an edit to an earlier comment and I relied on the auto-update of the github issue :)
I've dropped all other arches now, only the ones from typed-ast remain.

Signed-off-by: Jonas Jelten <jj@sft.mx>
Signed-off-by: Jonas Jelten <jj@sft.mx>
Closes: https://bugs.gentoo.org/662406
Signed-off-by: Jonas Jelten <jj@sft.mx>
@TheJJ
Copy link
Contributor Author

TheJJ commented Jan 31, 2019

done :) sorry for my mess :D

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2019-01-31 20:40 UTC
Newest commit scanned: 1c68ec3
Status: ✅ good

No issues found

@ghost
Copy link

ghost commented Feb 1, 2019

It looks good, but I get a test failure on astroid on py37 (pytest 4.1.1). Don't you get a failure?

@ghost
Copy link

ghost commented Feb 1, 2019

Oh no... I pushed the commits by mistake! Oh well, that becomes my problem. I'll investigate the cause of the test failures.

@gentoo-bot gentoo-bot closed this in bcd7942 Feb 1, 2019
@TheJJ
Copy link
Contributor Author

TheJJ commented Feb 1, 2019

Thank you :)

@TheJJ TheJJ deleted the pylint-v2 branch February 1, 2019 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR.
Projects
None yet
3 participants