search

conda / conda-lock

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

Parse Platforms Before Determining Dependencies #374

Closed srilman closed 1 year ago

srilman commented 1 year ago

Description

Closes #337. This PR changes _parse_source_files to first determine the list of platforms to render for by reading the source files. It will only do so if an explicit list of platforms have not been passed in.

netlify[bot] commented 1 year ago

Deploy Preview for conda-lock ready!

Name Link
Latest commit eb8001560400b94ed8ad10d681e9fda350220299
Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/63fbe7012c7bf90008774c0b
Deploy Preview https://deploy-preview-374--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.

srilman commented 1 year ago

Tried about 3 times and keep getting the md5 error, so I'm going to retry much later.

maresb commented 1 year ago

This is awesome!!! And thanks to your commit structure I believe I understand it.

I edited your description to change "Solves" → "Closes" since GitHub recognizes the "closes" keyword and automatically links the issue.

Sorry about the md5, I'm actually very close to solving it in #348. It seems to be some race condition where it can be overcome by repeating the dry-run solve.

For a current workaround, I'm able to rerun individual tests after clicking "Details".

image

But maybe you don't have the same privileges. (I'm not sure what's involved in granting those...)

maresb commented 1 year ago

The tests are green, but I force-pushed to reset your retry commits, and it restarted the tests. Now I'm blocked from merging until the required tests pass, but I'll merge tomorrow as soon as I can.

(If need be I'll restart the tests myself until they pass.)

maresb commented 1 year ago

If you get the chance it'd be really nice to clean up some of this code with a refactoring commit while it's still fresh in our minds.

The code seems fairly awkward which we might be able to clean up with something like a SourceFile class with a uniform interface. Also, it'd be nice if the source file type were detected by content rather than filename.

srilman commented 1 year ago

@maresb Thanks for handling all this!

For a current workaround, I'm able to rerun individual tests after clicking "Details".

Yeah I think you need special privileges to do that.

The code seems fairly awkward which we might be able to clean up with something like a SourceFile class with a uniform interface.

Yeah I felt so too. Using a SourceFile class would make it a bit neater by parsing each file once and rendering output when necessary. This is my plan for next PRs

Also, it'd be nice if the source file type were detected by content rather than filename.

Good point. I think we can assume that the file extensions are correct (YAML files end with .yml or .yaml and TOML files end with .toml) then:

maresb commented 1 year ago

Thanks a lot for the contribution. Your plan looks excellent! :rocket:

maresb commented 1 year ago

Possibly related, though possibly just a problem with micromamba: https://github.com/mamba-org/mamba/issues/1209#issuecomment-1447546266

CC @mfisher87

Edit: Nevermind, here we are talking about source files, while that is talking about lock files.