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

Store lockfile-relative sources in lockfiles #328

Merged

Conversation

riccardoporreca
Copy link
Contributor

Description

This is the revised fix for #229 following the original PR #230, especially given what done in #204 for the inputs metadata.

In fact, we use consistently what introduced in #204 for the actual metadata.sources in the lockfile, where lockfile-relative paths are always used (as the most sensible approach) unless this is made impossible by paths being on different drives, thus addressing the needs of #175, in a ways even more specific than what originally though in #228

* This was broken in conda#189, which introduced absolute source paths, and will be handled in conda#229.
* Fixes conda#229, relying on what introduced in conda#204 for the input metadata, by explicitly handling only the case of different drives for keeping absolute paths, addressing conda#175.
* Addressing the inconsistency with the absolute source paths introduced in conda#175 and noticed in conda#229.
@riccardoporreca
Copy link
Contributor Author

I introduced a test for the lockfile-relative sources. I am not sure how we can test the case where paths are on different drives, which is now the only case when absolute paths are stored. Happy to take any suggestion, perhaps mocking conda_lock.common.relative_path to raise the exception thus forcing the behavior?

@riccardoporreca
Copy link
Contributor Author

riccardoporreca commented Feb 4, 2023

There is an issue with the pre-commit, which I also had locally and seems completely unrelated

UPDATE: most likely, this is related to #327

@riccardoporreca
Copy link
Contributor Author

riccardoporreca commented Feb 4, 2023

There is also a failing test_run_lock_regression_gh155[micromamba] (on Python 3.8 only), which seems pretty unrelated as it should be very specific to #155

There seems to be "random" failures KeyError: 'md5' for different tests, which I guess might be unrelated/temporary

@maresb
Copy link
Contributor

maresb commented Feb 5, 2023

Ya, the precommit failure is related to a bad release of isort, and is fixed by #327. For the CI failures they are random and I am rerunning the failed tests. I am trying to track down the root cause but unfortunately my time is very limited over the next few weeks.

@riccardoporreca
Copy link
Contributor Author

@maresb, I noticed the merge fro main has also included dropping support for python < 3.8 (#330).

There are just a few conditions like if sys.version_info < (3, 8) in test_conda_lock.py, one of which is part of the test I introduced in the PR: I am happy to clean this up if you agree, otherwise it might be worth a second PR after this one is merged

@maresb
Copy link
Contributor

maresb commented Feb 7, 2023

Ya, I think it's fine to wait, especially since the cases you mention are so clearly marked. If someone comes along with an objection before the next release I am open to reverting #330, so I am glad to retain 3.6 compatibility for now.

At some point we should modernize the coding style, like by adding pyupgrade as a pre-commit hook. But before that I am hoping that we can wind down the review queue enough in order to avoid creating nasty merge conflicts for the current contributors.

Copy link
Contributor

@maresb maresb left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your patience and persistence on this. @mariusvniekerk, could you please take a look?

@maresb maresb requested a review from a team as a code owner February 25, 2023 00:15
@netlify
Copy link

netlify bot commented Feb 25, 2023

Deploy Preview for conda-lock ready!

Name Link
🔨 Latest commit c7fe62e
🔍 Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/63f95340ff6a200008e62fe1
😎 Deploy Preview https://deploy-preview-328--conda-lock.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@maresb maresb merged commit 1a87a6a into conda:main Feb 25, 2023
@maresb
Copy link
Contributor

maresb commented Feb 25, 2023

Thank you @riccardoporreca!!!

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.

None yet

3 participants