GNOME Bugzilla – Bug 777993
wip/chergert/rustup: the rustup plugin is not included in plugins/Makefile.am
Last modified: 2017-02-18 20:30:55 UTC
Hi, this is my first contribution to gnome-builder and to gnome in general. It's no big deal i see it more as an exercise to get used to bugzilla, etc.. While trying to build the wip/chergert/rustup branch, albfan on irc discovered that the plugins/Makefile.am was missing the rustup plugin directory. The patch fixes that. Thanks!
Created attachment 344657 [details] [review] add rustup to plugins The current wip rustup branch is missing to include the rustup plugin directory to Makefile.am
Review of attachment 344657 [details] [review]: We don't really use code-review/patches for wip branches. If you'd like to work on the branch, just clone it to your own branch and make progress there.
(In reply to Christian Hergert from comment #2) > Review of attachment 344657 [details] [review] [review]: > > We don't really use code-review/patches for wip branches. If you'd like to > work on the branch, just clone it to your own branch and make progress there. Okay, then you can close this bug. I don't know if this is the right place, but i would like to work on to integrate rustup. Do you have plans or an idea how to integrate it? What would be the next steps? Thank you in advance!
(In reply to georg.vienna from comment #3) > Okay, then you can close this bug. > I don't know if this is the right place, but i would like to work on to > integrate rustup. Do you have plans or an idea how to integrate it? > What would be the next steps? I think the next things to make work on that branch are: 1) Make the shell script (which was taken from rustup upstream) support progress when not using a TTY for output (such as that from GSubprocess). 2) Parse the progress output into a percentage for the IdeTransfer implementation (so we get progress while downloading). 3) Test that the whole thing works :)
I did a little bit of research and write it here for further discussion, hope you don't mind: The whole rustup process changed a little bit since then. The official way to install rust is using: curl https://sh.rustup.rs -sSf | sh which downloads and executes a script which will determine the architecture and download the rustup-init program for this architecture, which will then install rustup (and optionally the default toolchain). With rustup installed the user can then install different rust toolchains rustup toolchain install <channel>[-<date>][-<host>] <channel> = stable|beta|nightly|<version> <date> = YYYY-MM-DD <host> = <target-triple> If multiple toolchains are installed the user can switch toolchains by setting a new default: rustup default <toolchain> It is also possible run a command in an environment configured for a given toolchain: rustup run <toolchain> cargo which could be used to set a toolchain per project... So this brings up a few questions. Which and how should these parts be done by the rustup plugin? Install rustup: Should the plugin assume that rustup is installed and if not point to the rust-lang website OR should the plugin install rustup by either using the shell script or by simply downloading the right rustup-init program for the current architecture? Handling toolchains: Should the plugin handle multiple toolchains or just take the default one? And should the plugin allow to install/uninstall toolchains or to set the default toolchain? Btw. how can I get the transfer manager inside a preference addin?
I absolutely do not want us downloading shell scripts (https or not) to execute. If we are going to use rustup, we need to download a copy of the script and ship it with the plugin. I *might* consider certificate pinning and downloading the script, but even so I'm not thrilled about it because then we have to be on top of our game in terms of updating potentially revoked certificates.
(In reply to Christian Hergert from comment #6) > I absolutely do not want us downloading shell scripts (https or not) to > execute. If we are going to use rustup, we need to download a copy of the > script and ship it with the plugin. > > I *might* consider certificate pinning and downloading the script, but even > so I'm not thrilled about it because then we have to be on top of our game > in terms of updating potentially revoked certificates. The script only checks the architecture, downloads the right rustup-init executable and executes it. I think it would be easier to do this steps in the plugin, that way we have total control of the download and don't have to parse the output of the shell script.
Created attachment 344877 [details] [review] rustup plugin: updated rustup.sh, handle installation of rustup The rustup installation is detected and displayed in the preferences. If rustup is not installed, it is possible to install rustup via the plugin.
The patch is a starting point, for now it can detect a rustup installation and install a new rustup. Update is not yet implemented as i could never test it. Since i'm new to gnome and gnome-builder i am thankfull for any review comments. In the next days I will add more functionality to plugin.
(In reply to georg.vienna from comment #7) > The script only checks the architecture, downloads the right rustup-init > executable and executes it. I think it would be easier to do this steps in > the plugin, that way we have total control of the download and don't have to > parse the output of the shell script. I agree that doing this in a script without using upstream shell script is a good idea if it helps us provide a better experience.
Review of attachment 344877 [details] [review]: Excellent work on your first contribution! I'm impressed at how quickly you've gotten started and made progress using our Ide APIs. ::: plugins/rustup/rustup_plugin/__init__.py @@ +91,3 @@ """ + try: + ret = subprocess.check_output(['rustup', '-V']) I wouldn't expect you to know this up front, but we probably cannot use the Python subprocess module because we might want to execute the binary on the host. If we are running Flatpak (or any other containment system), we need to escape the sandbox to execute on the host. In Builder, IdeSubprocessLauncher will handle this for us if we set "launcher.props.run_on_host = True". Something like: launcher = Ide.SubprocessLauncher.new(Gio.SubprocessFlags.STDOUT_PIPE) launcher.push_argv('cargo') launcher.set_run_on_host(True) subprocess = launcher.spawn() _, stdout, stderr = subprocess.communicate_utf8(None, None) * Not actually tested, API may be slightly different. @@ -106,3 +160,3 @@ rustup_sh_path = get_module_data_path('resources/rustup.sh') - prefix = os.path.expanduser(self.props.prefix) - + # ensure that the script is executable + subprocess.check_output(['chmod', 'u+x', rustup_sh_path]) os.chmod()?
(In reply to Christian Hergert from comment #11) > Review of attachment 344877 [details] [review] [review]: > > Excellent work on your first contribution! I'm impressed at how quickly > you've gotten started and made progress using our Ide APIs. Thank you! The APIs are easy to use once you get the main principles and even tough the documentation is a little bit rare, the code is clean and easy to read! > > ::: plugins/rustup/rustup_plugin/__init__.py > @@ +91,3 @@ > """ > + try: > + ret = subprocess.check_output(['rustup', '-V']) > > I wouldn't expect you to know this up front, but we probably cannot use the > Python subprocess module because we might want to execute the binary on the > host. If we are running Flatpak (or any other containment system), we need > to escape the sandbox to execute on the host. In Builder, > IdeSubprocessLauncher will handle this for us if we set > "launcher.props.run_on_host = True". > > Something like: > > launcher = Ide.SubprocessLauncher.new(Gio.SubprocessFlags.STDOUT_PIPE) > launcher.push_argv('cargo') > launcher.set_run_on_host(True) > subprocess = launcher.spawn() > > _, stdout, stderr = subprocess.communicate_utf8(None, None) > > * Not actually tested, API may be slightly different. Done > > @@ -106,3 +160,3 @@ > rustup_sh_path = get_module_data_path('resources/rustup.sh') > - prefix = os.path.expanduser(self.props.prefix) > - > + # ensure that the script is executable > + subprocess.check_output(['chmod', 'u+x', rustup_sh_path]) > > os.chmod()? Didn'T knew :-) Changed This version of the patch contains the changes, handles external changes (so if the user installs or uninstalls rustup outside of gnome-builder we'll check it once a gnome-builder window gets active again) and allows to update the rustup installation. The next steps will be to display/change the currently installed toolchains and to install new toolchains inside the plugin
Created attachment 344971 [details] [review] rustup plugin: updated rustup.sh, handle install and update of rustup The rustup installation is detected and displayed in the preferences. If rustup is not installed, it is possible to install rustup, if it is installed it can be updated via the plugin.
Review of attachment 344971 [details] [review]: Nice progress! I'm going to try to merge the transfer manager improvements to master today which can help simplify this a bit (so you don't need to watch ::transfer-completed) ::: plugins/rustup/rustup_plugin/__init__.py @@ +26,3 @@ +import pty +import stat +import subprocess I think you can drop this import now? @@ -71,2 +97,2 @@ """ - settings = Gio.Settings.new('org.gnome.builder.plugins.rustup') + try: This might be a bit more succinct if you loop through some potential executable paths like: maybe_rustup = ['rustup', os.path.expanduser('~/.cargo/bin/rustup')] for rustup in maybe_rustup: launcher = ... @@ -72,4 +98,45 @@ - - channel = settings.get_string('channel') - prefix = settings.get_string('prefix') - settings = Gio.Settings.new('org.gnome.builder.plugins.rustup') + try: + launcher.push_argv('rustup') + def transfer_completed(self, transfer_manager, transfer): ... 42 more ... We can probably simplify this once the new build pipeline branch lands on master, because it contains an Ide.TransferManager.execute_async()/finish pair that implicitly performs the queue() and completes when the transfer fails/succeeds. @@ -72,4 +98,54 @@ - - channel = settings.get_string('channel') - prefix = settings.get_string('prefix') - settings = Gio.Settings.new('org.gnome.builder.plugins.rustup') + try: + launcher.push_argv('rustup') + # run it in all transfer managers ... 51 more ... We should only run this in one transfer manager or bad things will happen. Possibly an argument to make the transfer manager owned by the IdeApplication and not the IdeContext, hrmm..
(In reply to Christian Hergert from comment #14) > Review of attachment 344971 [details] [review] [review]: > > Nice progress! > > I'm going to try to merge the transfer manager improvements to master today > which can help simplify this a bit (so you don't need to watch > ::transfer-completed) > > ::: plugins/rustup/rustup_plugin/__init__.py > @@ +26,3 @@ > +import pty > +import stat > +import subprocess > > I think you can drop this import now? Yes you're right. > > @@ -71,2 +97,2 @@ > """ > - settings = Gio.Settings.new('org.gnome.builder.plugins.rustup') > + try: > > This might be a bit more succinct if you loop through some potential > executable paths like: > > maybe_rustup = ['rustup', os.path.expanduser('~/.cargo/bin/rustup')] > for rustup in maybe_rustup: > launcher = ... > Indeed! But anyway I hope they will fix handling the PATH in rustup, currently it is only set in .profile, which is why I look at the specific path. > @@ -72,4 +98,45 @@ > - > - channel = settings.get_string('channel') > - prefix = settings.get_string('prefix') > - settings = Gio.Settings.new('org.gnome.builder.plugins.rustup') > + try: > + launcher.push_argv('rustup') > + def transfer_completed(self, transfer_manager, transfer): > ... 42 more ... > > We can probably simplify this once the new build pipeline branch lands on > master, because it contains an Ide.TransferManager.execute_async()/finish > pair that implicitly performs the queue() and completes when the transfer > fails/succeeds. > That would be great. There is also a Bug if i use multiple transfer manager, where the progressbar gets not hidden on the second transfer manager, even tough the transfer was cancelled. But it will be probably fixed once there is an application transfer manager. > @@ -72,4 +98,54 @@ > - > - channel = settings.get_string('channel') > - prefix = settings.get_string('prefix') > - settings = Gio.Settings.new('org.gnome.builder.plugins.rustup') > + try: > + launcher.push_argv('rustup') > + # run it in all transfer managers > ... 51 more ... > > We should only run this in one transfer manager or bad things will happen. > Possibly an argument to make the transfer manager owned by the > IdeApplication and not the IdeContext, hrmm.. Yes something like that wojld be great, it is currently a little cumbersome to handle all open transfer manager.
Created attachment 346054 [details] [review] rustup plugin: handle install and update of rustup and rust toolchains The rustup installation is detected and displayed in the preferences. If rustup is not installed, it is possible to install rustup, if it is installed it can be updated via the plugin. Toolchains are listed, they can be installed, removed and the default toolchain can be set.
I finally managed to get back to the rustup plugin. It is now possible to manage rust toolchain through the plugin. The transfers are still handled by the applicationaddin, this way they are shown in all workbenches (contexts), but it would be the best if there is an application wide transfermanager, as you said before. I'm not sure how the '::transfer-completed' step can be simplified, maybe I missed something... Also I saw the new IdeTransferButton class, but it has not yet all the features I need. I could try to improve it so that it can be used in the plugin instead of the current solution if nobody else is working on it.
Great work! I've pushed this with various cleanups to master, so it will be part of 3.23.92 when that gets released. Sorry it took so long for me to review this, it was sort of a mad-dash to land the build pipeline as early as possible. I altered the UI a bit to be more consistent with other Builder UI. Attachment 346054 [details] pushed as bea8f55 - rustup plugin: handle install and update of rustup and rust toolchains