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

Fix channels_remap handling in extra_envs configuration #808

Merged
merged 15 commits into from
Jun 22, 2024

Conversation

jaimergp
Copy link
Contributor

@jaimergp jaimergp commented Jun 20, 2024

Description

An oversight in the strip logic does not handle channel_remaps correctly (it assumed list of str, but we have list of dict[str, str] there).

This input file:

...
extra_envs:
  napari-0.5.0a2.dev751+g3b0612ec:
    channels:
    - https://github.com/napari/pins/releases/download/napari-v0.5
    channels_remap:  # THIS IS THE PROBLEM
    - src: https://github.com/napari/pins/releases/download/napari-v0.5
      dest: napari-v0.5.0a2.dev751+g3b0612ec
    specs:
    - python=3.9.*=*_cpython
    - napari=0.5.0a2.dev751+g3b0612ec
    - napari-menu=0.5.0a2.dev751+g3b0612ec
    - napari-plugin-manager
    - pyside2=*
    - conda
    - mamba
    - pip
    menu_packages:
    - napari-menu
...

results in the following error:

Traceback (most recent call last):
  File "/home/runner/micromamba/envs/build-bundle/bin/constructor", line 10, in <module>
    sys.exit(main())
  File "/home/runner/micromamba/envs/build-bundle/lib/python3.9/site-packages/constructor/main.py", line 424, in main
    main_build(dir_path, output_dir=out_dir, platform=args.platform,
  File "/home/runner/micromamba/envs/build-bundle/lib/python3.9/site-packages/constructor/main.py", line 153, in main_build
    env_config[config_key] = [val.strip() for val in value]
  File "/home/runner/micromamba/envs/build-bundle/lib/python3.9/site-packages/constructor/main.py", line 153, in <listcomp>
    env_config[config_key] = [val.strip() for val in value]
AttributeError: 'CommentedMap' object has no attribute 'strip'

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?

@jaimergp jaimergp requested a review from a team as a code owner June 20, 2024 16:06
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Jun 20, 2024
if config_key == "environment_file":
env_config[config_key] = abspath(join(dir_path, value))
elif config_key == "channels_remap":
env_config[config_key] = [
{"src": item["src"].strip(), "dest": "dest".strip()} for item in value
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{"src": item["src"].strip(), "dest": "dest".strip()} for item in value
{"src": item["src"].strip(), "dest": item["dest"].strip()} for item in value

The way this code is written, it looks like the dest will always be "dest". I'm surprised the tests pass - sounds like the example is maybe not as representative as they should be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, good catch. This would only be caught if we try to update the resulting installation or run conda list on them because that's where the remapped channels would show up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extended a test

constructor/main.py Show resolved Hide resolved
CONSTRUCT.md Show resolved Hide resolved
constructor/construct.py Show resolved Hide resolved
@jaimergp jaimergp merged commit 319633c into conda:main Jun 22, 2024
18 checks passed
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
Status: 🏁 Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants