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: read feedstock name from rattler-build feedstock #2003

Merged
merged 17 commits into from
Aug 5, 2024

Conversation

nichmor
Copy link
Contributor

@nichmor nichmor commented Jul 30, 2024

Checklist

  • Added a news entry
  • Regenerated schema JSON if schema altered (python conda_smithy/schema.py)

Fix invoking conda-smithy register-github and conda-smithy register-ci when rattler-build recipe.yaml is used in feedstock.

@nichmor nichmor marked this pull request as ready for review July 30, 2024 15:18
@nichmor nichmor requested a review from a team as a code owner July 30, 2024 15:18
tests/test_utils.py Outdated Show resolved Hide resolved
@nichmor nichmor changed the title fix: use right feedstock name fix: read feedstock name from rattler-build feedstock Jul 30, 2024
@nichmor
Copy link
Contributor Author

nichmor commented Jul 30, 2024

@beckermr @jaimergp

conda_smithy/utils.py Outdated Show resolved Hide resolved
nichmor and others added 2 commits July 30, 2024 18:27
Co-authored-by: Bas Zalmstra <zalmstra.bas@gmail.com>
based on conda_build_tool value from forge_config.
Raises OsError if no meta.yaml or recipe.yaml is found in feedstock_directory.
"""
if forge_config and forge_config.get("conda_build_tool") == RATTLER_BUILD:
Copy link
Member

Choose a reason for hiding this comment

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

Will this work with a fresh feedstock? Because I am not sure if we will have the right forge_config.

Copy link
Member

Choose a reason for hiding this comment

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

@nichmor did you see this comment?

Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

No api changes.

@baszalmstra
Copy link
Member

@beckermr Can you elaborate what you mean by this? Are you refering to the move of the private functions? Or did we miss something?

@beckermr
Copy link
Member

Only functions that start with an underscore are private in python. Anything else is a public api and cannot be moved or renamed.

@baszalmstra
Copy link
Member

I thought thats exactly what we did? Did we accidentally move non-underscorred functions?

conda_smithy/cli.py Outdated Show resolved Hide resolved
conda_smithy/utils.py Show resolved Hide resolved
Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

Upper bound should be ",<2.0.0a0".

@nichmor
Copy link
Contributor Author

nichmor commented Aug 2, 2024

Upper bound should be ",<2.0.0a0".

changed

environment.yml Outdated Show resolved Hide resolved
Co-authored-by: Matthew R. Becker <beckermr@users.noreply.github.com>
Copy link
Member

@isuruf isuruf left a comment

Choose a reason for hiding this comment

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

Can we keep the functions you moved to utils.py in configure_feedstock.py? It doesn't make sense to me to move them to a different file.

@baszalmstra
Copy link
Member

Can we keep the functions you moved to utils.py in configure_feedstock.py? It doesn't make sense to me to move them to a different file.

In not sure I understand. In trying to understand this better. From my perspective _load_forge_config is the only function that is now used in configure_feedstock and it is used in a few different other functionality with this PR. Why doesnt it make sense to have this common functionality in a seperate file? What makes this tightly coupled to configure_feedstock?

@isuruf
Copy link
Member

isuruf commented Aug 2, 2024

I missed that. However _load_forge_config is a configuration file for the feedstock and belongs in configure_feedstock. It does not make sense at all to use utils.py which is a generic name. Why not import from conda_smithy.configure_feedstock?

@nichmor
Copy link
Contributor Author

nichmor commented Aug 2, 2024

Can we keep the functions you moved to utils.py in configure_feedstock.py? It doesn't make sense to me to move them to a different file.

Hey @isuruf !
We introduced the utility function get_feedstock_name_from_metadata which I've decided to keep in utils.py as it's not tightly coupled to configure_feedstock.py.
Unfortunately while trying to import other utility functions from configure_feedstock.py I had a circular import, so I've decided to put them in the utils module, as it more makes sense based on their responsibility

@isuruf
Copy link
Member

isuruf commented Aug 2, 2024

Unfortunately while trying to import other utility functions from configure_feedstock.py I had a circular import, so I've decided to put them in the utils module, as it more makes sense based on their responsibility

utils module makes sense based on the responsibility? utils module doesn't have any responsibility. It's a bad design to move functionality to a generic utils name.

@isuruf
Copy link
Member

isuruf commented Aug 2, 2024

@baszalmstra
Copy link
Member

Sure, I agree! But maybe we can just think of a better name instead? conda_forge_config for instance? To work around circular import issue.

@isuruf
Copy link
Member

isuruf commented Aug 2, 2024

Where is the circular import coming from? You can import inside a function instead of at a top-level to avoid circular imports.

@nichmor
Copy link
Contributor Author

nichmor commented Aug 5, 2024

hey @isuruf ! I've reverted the files in their old locations

@nichmor nichmor requested review from beckermr and isuruf August 5, 2024 12:22
conda_smithy/utils.py Outdated Show resolved Hide resolved
@nichmor
Copy link
Contributor Author

nichmor commented Aug 5, 2024

@isuruf removed the indirection

Co-authored-by: Bas Zalmstra <zalmstra.bas@gmail.com>
@nichmor nichmor requested a review from isuruf August 5, 2024 13:33
@beckermr
Copy link
Member

beckermr commented Aug 5, 2024

LGTM from me but we should wait for Isuru.

@beckermr beckermr merged commit b8b75d0 into conda-forge:main Aug 5, 2024
2 checks passed
@beckermr beckermr mentioned this pull request Aug 5, 2024
2 tasks
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.

5 participants