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

Default platforms are unexpectedly added from multiple sources #337

Closed
2 tasks done
maresb opened this issue Feb 9, 2023 · 2 comments · Fixed by #374
Closed
2 tasks done

Default platforms are unexpectedly added from multiple sources #337

maresb opened this issue Feb 9, 2023 · 2 comments · Fixed by #374

Comments

@maresb
Copy link
Contributor

maresb commented Feb 9, 2023

Checklist

  • I added a descriptive title
  • I searched open reports and couldn't find a duplicate

What happened?

If I use both pyproject.toml and a supplemental environment.yml as sources, and if I specify platforms in pyproject.toml but not in environment.yml, I would expect the resulting platforms to be the list from pyproject.toml. Instead it's those platforms union the default platforms (linux-64, osx-64, win-64).

The logic in parse_source_files looks odd to me. It seems like we add the default platforms if they are missing from any source file (except for pyproject.toml?).

I would have expected that we take the union of all platforms from all source files, and then substitute the default platforms only if that were empty.

Ping @srilman since this involves code we've been looking at recently.

Conda Info

No response

Conda Config

No response

Conda list

No response

Additional Context

No response

@srilman
Copy link
Contributor

srilman commented Feb 9, 2023

I agree, this is really odd behavior. I think this is because for Conda source files like meta.yaml and environment.yml files, we need to know the list of platforms before we actually start parsing the file in order for OS preprocessor selectors to work correctly. However, as I think you suggested in #278 (comment), what we should really do is parse the source files once to get the platforms, and then parse them again to get the list of dependencies.

@srilman
Copy link
Contributor

srilman commented Feb 9, 2023

Since we were planning to split #316 into smaller pieces anyways, I think I can do the following to get this problem fixed first:

  • Take out the first 2 commits and move to a separate PR. You mentioned that you already reviewed them, so it should be easy to get the new PR merged.
  • Open a PR to fix this specific behavior, without using a new SourceFile class
  • Then rebase and split New Source File and Lock Specification Approach #316 further to make reviewing easier.

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 a pull request may close this issue.

2 participants