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

Prepare menuinst for multiplatform support #8

Merged
merged 23 commits into from
Aug 8, 2023
Merged

Conversation

jaimergp
Copy link
Contributor

@jaimergp jaimergp commented Oct 14, 2021

Standardizing the changes added in menuinst@cep-devel for other implementations.

Please refer to the rendered version of the proposal to read comfortably :D

@jaimergp jaimergp changed the title Formalize schema for menuinst Prepare menuinst for multiplatform support Oct 15, 2021
@ericpre
Copy link

ericpre commented Oct 17, 2021

Thanks @jaimergp, this looks very promising.
From working on conda-forge/conda-standalone-feedstock#18, your summary in https://gist.github.com/jaimergp/7de5843421d63fa4a408ac5c8712c3c9 and the current suggested actions of this PR looks very good to me, as this would disentangle the menuinst/conda-standalone/constructor situation and simplify adding new feature and making release!

Two suggestions to consider:

@jaimergp
Copy link
Contributor Author

jaimergp commented Oct 18, 2021

Thanks Eric! I'll add in your suggestions!

Add pkg_names argument to menuinst

This would concern only the install_all wrapper, I assume, right? In that case I guess the filter option could be invoked from the caller (constructor in this case), if needed.

@jaimergp jaimergp marked this pull request as ready for review October 18, 2021 11:44
@jaimergp
Copy link
Contributor Author

@conda/conda-core this is ready for review, I'd say!

cep-9999.md Outdated Show resolved Hide resolved
@wolfv
Copy link
Contributor

wolfv commented Oct 19, 2021

I think this is great! Thanks for the effort & reviving menuinst. I am excited about macOS and Linux support.

@jaimergp
Copy link
Contributor Author

@chenghlee I remember you mentioned some team at Anaconda was also looking into this. Can you let them know about this CEP and maybe share feedback? Thanks!

Copy link

@ocefpaf ocefpaf left a comment

Choose a reason for hiding this comment

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

@jaimergp all looks good but I don't really have any skin in the game here. Just happy to see a feature that is loved by many revitalized.

@ericpre
Copy link

ericpre commented Oct 26, 2021

Thanks Eric! I'll add in your suggestions!

Add pkg_names argument to menuinst

This would concern only the install_all wrapper, I assume, right? In that case I guess the filter option could be invoked from the caller (constructor in this case), if needed.

Yes, I think so!

@ericpre
Copy link

ericpre commented Oct 26, 2021

A comment related to #8 (comment): if I understand correctly - using menu items in packages requires the recipe not to be noarch, because selectors are necessary to tell conda-build where to take the file to include in the package. If there was a single file instead of one for each platform, then this would be simplified and this requirement will not be necessary.

@jaimergp
Copy link
Contributor Author

Question: How to do cross-platform activation?

  • Currently, on Windows, menuinst has cwp.py, which pseudo-activates the environment. It massages PATH and then launches specified command.
    • Users call it like PREFIX/python PREFIX/cwp.py PREFIX command args ...
  • On Unix-like platforms, this might not be enough because we also need LD_LIBRARY_PATH :/ We can use the same Python-depending approach and massage env vars and then relaunch in a subprocess.

However, this is not true activation at all. No post-activation scripts are executed, for example.

I was thinking of moving cwp.py to the menuinst namespace, maybe as menuinst.activate. So people can do PREFIX/python -m menuinst.activate -- command args. However, it requires Python and some projects might not use it for other stuff.

We can also expose this in the conda-standalone CLI so it fakes activation in each platform. But actually, conda run already does this for us! The only problem is that other self-contained implementations like micromamba would need to implement this as well to be constructor-compliant.

Thoughts? Maybe I am overthinking this, but the whole bootstrapping issue is hard to reason about 😬

Cc @ericpre @wolfv

@ericpre
Copy link

ericpre commented Nov 25, 2021

Sorry, I can't help on this, but I would be interested in the answer!

On a slightly different topic and maybe this is out of the scope of this CEP: is it worth adding an option to define if the menu shortcut is for all users or single user only? This could useful for example to address mamba-org/mamba#923.

@wolfv
Copy link
Contributor

wolfv commented Nov 26, 2021

@jaimergp is it feasible to create launch scripts (like python entrypoints)?

That could hard code the activation for a given command. E.g. the output of micromamba shell activate

Then the script could look like:

menuinst-napari.sh

#/usr/bin/env sh
. "/Users/wolfvollprecht/micromamba/etc/conda/deactivate.d/deactivate_clangxx_osx-arm64.sh"
. "/Users/wolfvollprecht/micromamba/etc/conda/deactivate.d/deactivate_clang_osx-arm64.sh"
. "/Users/wolfvollprecht/micromamba/etc/conda/deactivate.d/deactivate-gfortran_osx-arm64.sh"
PS1='(base) '
export PATH='/Users/wolfvollprecht/micromamba/bin:/Users/wolfvollprecht/micromamba/condabin:/Users/wolfvollprecht/.gem/ruby/3.0.0/bin:/opt/homebrew/opt/ruby/bin:/opt/homebrew/opt/ruby/gems/3.0.0/bin:/opt/homebrew/bin:/opt/homebrew/sbin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/share/dotnet:~/.dotnet/tools:/Library/Apple/usr/bin:/Library/Frameworks/Mono.framework/Versions/Current/Commands'
export CONDA_SHLVL='1'
export CONDA_PROMPT_MODIFIER='(base) '
. "/Users/wolfvollprecht/micromamba/etc/conda/activate.d/activate-gfortran_osx-arm64.sh"
. "/Users/wolfvollprecht/micromamba/etc/conda/activate.d/activate_clang_osx-arm64.sh"
. "/Users/wolfvollprecht/micromamba/etc/conda/activate.d/activate_clangxx_osx-arm64.sh"

$CONDA_PREFIX/bin/napari

?

@wolfv
Copy link
Contributor

wolfv commented Nov 26, 2021

We would need to rewrite all these scripts for every modification of hte environment to take any additional post-activation scripts into account.

@wolfv
Copy link
Contributor

wolfv commented Nov 26, 2021

Or we write the equivalent bash / zsh function to call all activation scripts ...

@jaimergp
Copy link
Contributor Author

On MacOS we are using bash scripts to launch the programs, so we can add as much logic as needed. We didn't need that yet because we were relying on python.app to do the activation logic. See here. On Linux, it's currently a line on the .desktop file but I believe that's still executed by the shell directly, so we can do that as well.

Basically, I'd like to be able to source or include shell code to initialize the environment before the requested command is run. Whether the code comes from a file ad-hoc we create on the environment or we inject it in the shortcut file is more of an implementation detail.

On Windows, there's no script file or shell. The shortcut will run that command directly (I don't know the exact mechanism). We pseudoactivate with python cwp.py. cwp.py is provided by menuinst so it looks that this is being injected by constructor regardless, along with the whole python stack? In other words, on Windows, Python was already expected to be part of the target environment. It's a pity because conda-standalone is a Python stack already, but we can't assume that's going to be always the case for other implementations like micromamba.

If we want to undo that requirement, we could use a cmd call to add more logic. However, that will have a hanging CMD console in the background for no reason, unless there's a flag we can set in the shortcut file to prevent that? For example, subprocess on Windows allows you to add a CREATE_NO_WINDOW flag. Maybe the shortcut can also be decorated with that flag? No clue.

@jaimergp
Copy link
Contributor Author

is it worth adding an option to define if the menu shortcut is for all users or single user only?

I think that should depend on the installation we are creating a shortcut for. I think there are .nonadmin files in Windows envs to mark this. We need to investigate how to detect this, but in my opinion it shouldn't be part of the metadata. At least that's how I am implementing it for now. Definitely worth investigating at a later stage, maybe after figuring out activation.

@goanpeca
Copy link

This is looking great @jaimergp ! 🥳 thanks for working on this.

What would be the next steps to get this moving forward?

Cheers!

@wolfv
Copy link
Contributor

wolfv commented Feb 22, 2022

I just read the bylaws and it sounds like you can start having a vote on the CEP:

Enhancement Proposal Approval: When ready, the proposer may call for a vote on an existing conda enhancement proposal (EP). This requires a super-majority (60%) to pass so that the decision to accept the EP is unequivocal and we have ensured that consensus has been reached.
Standard
60% Majority to pass

https://github.com/conda-incubator/governance

However, I am unsure which the mentioned core meetings are (maybe the conda community sync? maybe they don't exist right now?)

@jaimergp
Copy link
Contributor Author

jaimergp commented Jul 6, 2023

For the record, I plan to call a vote for this in two weeks (next conda community call), so if you have any comments before the vote, this is the time. Thanks! 🙏

@jaimergp
Copy link
Contributor Author

@conda-incubator/steering, I would like to call a vote for this CEP.

This vote falls under the "Enhancement Proposal Approval" policy of the conda governance policy,
please vote and/or comment on this proposal at your earliest convenience.

It needs 60% of the Steering Council to vote yes to pass.

To vote, please leave yes, no or abstain as comments below.

If you have questions concerning the proposal, you may also leave a comment or code review.

This vote will end on August 3rd, 2023 End of Day, Anywhere on Earth (AoE).

Copy link

@goanpeca goanpeca left a comment

Choose a reason for hiding this comment

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

yes

@jezdez
Copy link
Member

jezdez commented Jul 18, 2023

yes

1 similar comment
@wolfv
Copy link
Contributor

wolfv commented Jul 19, 2023

yes

@marcelotrevisani
Copy link
Member

yes

Copy link

@xhochy xhochy left a comment

Choose a reason for hiding this comment

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

Yes

@jaimergp
Copy link
Contributor Author

yes

cep-9999.md Outdated Show resolved Hide resolved
Copy link

@mariusvniekerk mariusvniekerk left a comment

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor

@msarahan msarahan left a comment

Choose a reason for hiding this comment

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

Yes

Copy link

@chenghlee chenghlee left a comment

Choose a reason for hiding this comment

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

Yes

@ocefpaf
Copy link

ocefpaf commented Aug 2, 2023

Yes

Copy link
Member

@mbargull mbargull left a comment

Choose a reason for hiding this comment

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

yes

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Yes

(guessing the filename will be updated with the CEP #)

@jaimergp
Copy link
Contributor Author

jaimergp commented Aug 7, 2023

Hello team! The vote concluded last week on Aug 3rd at 23:59 AOE (Aug 4th 11:59 UTC). I have now reviewed the votes and these are the results:

  • Yes: 12
  • No: 0
  • Abstain: 0
  • Available quorum: 14
  • Criteria: 60% of quorum
  • 12/14 = 85.7%
  • ✅ The vote has passed!

--

I will now update the PR to reflect the status and mint a CEP number. Since the vote for this PR was started earlier than #51, this has priority, and will be minted the number 10.

@jaimergp
Copy link
Contributor Author

jaimergp commented Aug 7, 2023

Ah wait, this is number 11. CEP 10 already exists but the ordering is messed up in repo list. We need some padding 0s I guess :P

@jaimergp jaimergp merged commit 3da0fb0 into conda:main Aug 8, 2023
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.