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

Parse Platforms Before Determining Dependencies #374

Merged
merged 4 commits into from
Feb 27, 2023

Conversation

srilman
Copy link
Contributor

@srilman srilman commented Feb 26, 2023

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.

@srilman srilman requested a review from a team as a code owner February 26, 2023 19:47
@netlify
Copy link

netlify bot commented Feb 26, 2023

Deploy Preview for conda-lock ready!

Name Link
🔨 Latest commit eb80015
🔍 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
Copy link
Contributor Author

srilman commented Feb 26, 2023

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

@maresb
Copy link
Contributor

maresb commented Feb 26, 2023

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
Copy link
Contributor

maresb commented Feb 26, 2023

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
Copy link
Contributor

maresb commented Feb 26, 2023

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.

@maresb maresb merged commit ec93a54 into conda:main Feb 27, 2023
@srilman
Copy link
Contributor Author

srilman commented Feb 27, 2023

@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

  • Refactor LockSpecification to represent dependencies as a dictionary between the platform and list of deps.
  • Refactor to use ruamel-yaml
  • Refactor to use a SourceFile class

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
Copy link
Contributor

maresb commented Feb 27, 2023

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

@maresb
Copy link
Contributor

maresb commented Feb 28, 2023

Possibly related, though possibly just a problem with micromamba: mamba-org/mamba#1209 (comment)

CC @mfisher87

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

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.

Default platforms are unexpectedly added from multiple sources
2 participants