这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@jaimergp
Copy link
Contributor

@jaimergp jaimergp commented Aug 24, 2023

Description

Old menuinst v1 import paths are still in use in conda.core.initialize. To avoid having to issue repodata patches for a lot of conda versions we are providing this little forwarding logic using apipkg (only on Windows).

I don't know how the team feels about adding one more dependency to the conda tree. If that's a problem we could vendor the apipkg project.

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?

@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Aug 24, 2023
@jaimergp jaimergp marked this pull request as ready for review August 24, 2023 16:41
@jaimergp jaimergp requested a review from a team as a code owner August 24, 2023 16:41
@jaimergp
Copy link
Contributor Author

Ok this should be ready to go @conda/constructor. I have two main questions:

  • Should we depend on apipkg or vendor it?
  • apipkg has a few caveats, the main one is that once invoked, the namespace in that module is completely CLEARED. The fix is to reinject them via the attr kwarg. It works ok after that, but it was surprising behaviour. I added a comment to clarify, but I want to know if everyone is comfortable with that.

Thanks to the forwarders, menuinst v2 is backwards compatible with all the public calls I could find on Github (mostly conda.core.initialize).

@jezdez
Copy link
Member

jezdez commented Aug 24, 2023

I don't know how the team feels about adding one more dependency to the conda tree. If that's a problem we could vendor the apipkg project.

I know that apipkg has been around for a LONG time, so it's probably pretty well tested and used, but I also remember that Holger suggested to always vendor it as it's relatively small.

@jaimergp
Copy link
Contributor Author

I also remember that Holger suggested to always vendor it as it's relatively small.

The README suggests is a single module, but all I see is a package with several (small) modules. Am I missing something? 🤔

@jaimergp
Copy link
Contributor Author

Ah, that's v3. Maybe I try v2, which is indeed single-module.

@jezdez
Copy link
Member

jezdez commented Aug 25, 2023

pytest-dev/apipkg@2acee50 seems to have changed that, I didn't know that before

@jezdez
Copy link
Member

jezdez commented Aug 25, 2023

@jaimergp Hmm, seeing that the v3 also added Python 3.11 support (or at least testing), I wonder if dependening might actually be better :-/ It exists both on defaults and conda-forge: https://anaconda.org/anaconda/apipkg https://anaconda.org/conda-forge/apipkg but the versions don't match.

@jaimergp
Copy link
Contributor Author

defaults is reaaally behind with 1.5.

We can vendor the whole v3 package too. Even if they are a few files. 🤷

@jaimergp jaimergp mentioned this pull request Aug 25, 2023
19 tasks
@jaimergp
Copy link
Contributor Author

WDYT about this way @jezdez?

@jezdez
Copy link
Member

jezdez commented Aug 25, 2023

@jaimergp that's alright, I think it might be a little easier to maintain when using pip's vendoring lib, but that can be done separately

@jezdez
Copy link
Member

jezdez commented Aug 25, 2023

(need to review the API here in depth, next week though)

jezdez
jezdez previously approved these changes Aug 31, 2023
@jaimergp
Copy link
Contributor Author

Solving the conflicts dismissed the review, sorry @jezdez. Can you reapprove when you have a chance? Thanks 🙏

@jaimergp jaimergp merged commit 78c4041 into conda:main Sep 3, 2023
@jaimergp jaimergp added this to the v2.0 milestone Sep 13, 2023
@github-actions github-actions bot added the locked [bot] locked due to inactivity label Sep 13, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed [bot] added once the contributor has signed the CLA locked [bot] locked due to inactivity

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants