conda / conda-lock

Lightweight lockfile for conda environments
https://conda.github.io/conda-lock/
Other
470 stars 102 forks source link

Store lockfile-relative sources in lockfiles #328

Closed riccardoporreca closed 1 year ago

riccardoporreca commented 1 year ago

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

riccardoporreca commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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.

netlify[bot] commented 1 year ago

Deploy Preview for conda-lock ready!

Name Link
Latest commit c7fe62ee8a4718a79c246c021a5ecf44dc634230
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 commented 1 year ago

Thank you @riccardoporreca!!!