Fix: reload and merge easy-install pth file before save

[gst]

Summary of changes

Refactor PthDistributions._load into/with a second _load_raw. Re-use _load from PthDistributions.save().

and thus use that in save() to reload/refresh the file content before saving. and merge the eventual new found items.

Closes #3126

[abravalheri]

Hi @gst thank you very much for the PR! Any help is very appreciated.

I probably lack the knowledge to properly review your PR, so I will leave that for the other maintainers.
But something that would really help is if you can add a test case capturing the failing behaviour you are trying to solve.

[gst]

Hi @abravalheri

I have got this :

# CUTTED

that I added in setuptools/tests/test_easy_install.py but I can't succeed to make it run right now..

I'm not used to run setuptools tests I guess ;)

[abravalheri]

Hi @gst.

Something that is useful to run is flake8 setuptools (pytest-flake8 errors are quite cryptic, so you can solve that first before running the tests).

If you have tox installed, I would suggest running tox -- -p no:cov first (coverage will add a lot of noise) and then if everything passes just tox. You can also add -x and the specific test file if the tests are too slow in your machine (e.g. tox -- -p no:cov -x setuptools/tests/test_easy_install.py).

[abravalheri]

Could you add a little comment/note in this test explaining in which scenario the PthDistributions would be duplicated?

        pth1 = PthDistributions(pth_path)
        pth2 = PthDistributions(pth_path)

(That is a way of signing for people changing this file in the future why this test should not be removed).

[abravalheri]

(By the way, thank you very much for working in a test case, that really helps in the PR and to better understand the problem).

[gst]

That's my pleasure :)

but I cannot get the test to succeed still.. I'm doube checking..

but it's not easy with tox I find. any direct python command that I could pdb with ?

[abravalheri]

but it's not easy with tox I find. any direct python command that I could pdb with ?

That is a tricky question.

I usually rely on pytest's ability to open pdb on failure, e.g.:

tox -- -p no:cov -x --pdb setuptools/tests/test_easy_install.py

If you want to avoid tox completely it is more complicated... I imagine you can create a virtualenv install .[testing] inside of it and then proceed running:

pytest -p no:cov -x --pdb setuptools/tests/test_easy_install.py

(I never tested this way of running the tests though).

[gst]

voila. could make it to succeed.

FWIW I could use :

pytest -p no:cov -vs -x --pdb setuptools/tests/test_easy_install.py::TestPTHFileWriter::test_many_pth_distributions_merge_together

after having uninstalled pytest-flake8 and flake8.

[gst]

Hi, any comment on the test case and the change/fix(or enhancement?) ?

[abravalheri]

Hi @gst, I will have a look on the test case, thank you very much!

Unfortunately, I will have to leave further review to other maintainers because this topic is a bit out of the areas of setuptools I am familiar with.

I you need these changes in your workflow urgently, there are a few workarounds that you can use while this PR gets reviewed. For example, in your pyproject.toml you can use:

[build-system]
requires = [
    "setuptools @ git+https://github.com/gst/setuptools@fix-easy-install-pth-file-not-reloaded-before-save",
    ...
]

[abravalheri]

Thank you very much @gst for working on this!

I added just some general code review comments. I am unable assess how this change might impact on existing code, or if there is any undesired side-effect for other use cases that might be caused by this change. So I will leave this part of the review for other maintainers.

Please correct me here if I am wrong: I am assuming this change is necessary to make editable installs (i.e. pip install -e) work correctly, right? (In general we want to avoid changing easy_install since it is deprecated, unless it is necessary to make other parts of setuptools work correctly).

Finally, I would like to say that my comments are just suggestions and I am aware that people have different coding styles :)
I also understand perfectly if you want to wait until you have feedback from other maintainers before spending more energy and add other changes to this PR (to make sure your efforts are not going to be wasted).

[abravalheri]

Currently, the CI seems to be unhappy.

I recommend always running flake8 setuptools and tox (the full test suite) before pushing.

Of course you just run pytest locally while working on the changes (so you can have quick feedback), but it is good to run tox once you are finished to make sure everything is in a good shape (and avoid unexpected CI errors).

[gst]

@abravalheri I made the first changes you proposed.

I ran tox locally and everything succeed but 2 test cases .. not related.

I'm waiting anymore comments or review.

[abravalheri]

Thank you very much @gst, that is really appreciated.

Just a minor comment: flake8 seems to be still unhappy 😅.

[gst]

@abravalheri that's flaked now ;)

[jaraco]

Overall, this change looks reasonable. I'm sorry the pytest-flake8 reporting is so bad (due to tholo/pytest-flake8#81). I'll try to review this soon.

[jaraco]

Well, this is annoying - I replaced pytest-flake8 with pytest-flake8-v2, and the errors are reported:

E           pytest_flake8.Flake8Error: Detected 3 errors.
E           
E           /Users/jaraco/code/public/pypa/setuptools/setuptools/command/easy_install.py:251:80: E501 line too long (85 > 79 characters)
E           /Users/jaraco/code/public/pypa/setuptools/setuptools/command/easy_install.py:252:80: E501 line too long (84 > 79 characters)
E           /Users/jaraco/code/public/pypa/setuptools/setuptools/command/easy_install.py:1587:80: E501 line too long (85 > 79 characters)

But the errors are all for lines shorter than 88 characters, which is the declared limit. I'll need to track down this discrepancy.

[jaraco]

I found tholo/pytest-flake8#22, which shows some users having difficulty with pytest-flake8 getting it to honor the settings, so maybe that's what's happening.

[gst]

should I do something to help to make it pass ?

[jaraco]

Nah. I'll deal with that separately.

[kayal28]

🥰🥰🥰

[kayal28]

💜💙❤️

[gst]

ping