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

Update conda inspect channels #5033

Merged
merged 15 commits into from
Dec 8, 2023

Conversation

kenodegard
Copy link
Contributor

@kenodegard kenodegard commented Oct 10, 2023

Description

Update conda inspect channels to use latest solver/transactions logic instead of legacy get_index/actions.

Xref conda/conda#13192

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@kenodegard kenodegard self-assigned this Oct 10, 2023
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Oct 10, 2023
@kenodegard kenodegard marked this pull request as ready for review October 10, 2023 13:52
jaimergp
jaimergp previously approved these changes Oct 10, 2023
Copy link
Contributor

@jaimergp jaimergp left a comment

Choose a reason for hiding this comment

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

LGTM but I've added some comments of things to double check because the conda-build<>conda interface for the solver is full of hacks (e.g. how subdirs are merged when fetching the index) and I don't know if those are assumed to apply here or not.

if channel != "defaults" and rec.get("schannel", "defaults") == "defaults":
continue
name = rec["name"]
for subdir in ["osx-64", "linux-32", "linux-64", "win-32", "win-64"]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this use KNOWN_SUBDIRS in conda.somewhere.constants?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered this, but opted to leave that as a separate exercise since the primary goal was to stop using legacy solve stuff, not to fix conda inspect channels. Ideally this should be a customizable field from the CLI.

conda_build/inspect_pkg.py Outdated Show resolved Hide resolved
match = has_py.search(build)
assert match if "py" in build else True, build
if match:
additional_packages = [f"python={match.group(1)}.{match.group(2)}"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did additional_packages go? Can you explain why is it being removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understood (and from my testing) this doesn't make a difference. The package already defines its Python dependency so theres no need for us to also define it here.

This was actually prompted by the removal of the regex search on 183. Packages that do not follow the pyNN build string pattern fails (e.g. https://anaconda.org/conda-test/itsdangerous/files).

See conda inpsect channels conda-test --test-installable.

Solver(
prefix,
channel_urls,
[subdir],
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this fail if subdir is None? I'd make sure the default value is compatible with the Solver class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I remember that conda-build would merge native_subdir, "noarch" in a single made-up subdir internally, so I don't know if this assumption was here too (in case we need to pass "noarch" here). I think it doesn't, but I'd double check just in case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since this didn't have any noarch handling before I opted to not dig any further

good catch on the None, I'll work on that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

subdir fallback: 9c1100f

channel_urls,
[subdir],
specs_from_args(packages),
).solve_for_transaction().print_transaction_summary()
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this output should be very similar to original one, so ok for me, but just mentioning that it might not be, in case that's important.

conda_build/inspect_pkg.py Outdated Show resolved Hide resolved
Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
conda_build/inspect_pkg.py Outdated Show resolved Hide resolved
conda_build/inspect_pkg.py Outdated Show resolved Hide resolved
jezdez
jezdez previously approved these changes Dec 7, 2023
@kenodegard kenodegard merged commit 37ab8d3 into conda:main Dec 8, 2023
24 checks passed
@kenodegard kenodegard deleted the update-conda-inspect-channels branch December 8, 2023 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants