This is an archive of the discontinued Mercurial Phabricator instance.

rhg: Add support for automatic fallback to Python
ClosedPublic

Authored by SimonSapin on Mar 2 2021, 2:58 PM.

Download Raw Diff

Details

Reviewers

Alphare
pulkit

Group Reviewers

hg-reviewers

Commits

rHG93e9f448273c: rhg: Add support for automatic fallback to Python

Summary

rhg is a command-line application that can do a small subset of what
hg can. It is written entirely in Rust, which avoids the cost of starting
a Python interpreter and importing many Python modules.
In a script that runs many hg commands, this cost can add up.

However making users decide when to use rhg instead of hg is
not practical as we want the subset of supported functionality
to grow over time.

Instead we introduce "fallback" behavior where, when rhg encounters
something (a sub-command, a repository format, …) that is not implemented
in Rust-only, it does nothing but silently start a subprocess of
Python-based hg running the same command.
That way rhg becomes a drop-in replacement for hg that sometimes
goes faster. Whether Python is used should be an implementation detail
not apparent to users (other than through speed).

A new fallback value is added to the previously introduced
rhg.on-unsupported configuration key. When in this mode, the new
rhg.fallback-executable config is determine what command to use
to run a Python-based hg.

The previous rhg.on-unsupported = abort-silent configuration was designed
to let a wrapper script call rhg and then fall back to hg based on the
exit code. This is still available, but having fallback behavior built-in
in rhg might be easier for users instead of leaving that script "as an
exercise for the reader".

Using a subprocess like this is not idea, especially when rhg is to be
installed in $PATH as hg, since the other hg.py executable needs
to still be available… somewhere. Eventually this could be replaced
by using PyOxidizer to a have a single executable that embeds a Python
interpreter, but only starts it when needed.

Diff Detail

Repository

rHG Mercurial

Branch

default

Lint

No Linters Available

Unit

No Unit Test Coverage

Event Timeline

SimonSapin created this revision.Mar 2 2021, 2:58 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptMar 2 2021, 2:58 PM

Herald added a subscriber: mercurial-patches. · View Herald Transcript

✅ refresh by Heptapod after a successful CI run (🐙 💚)

SimonSapin added a child revision: D10094: rhg: Make fallback to Python the default behavior.Mar 3 2021, 12:08 PM

SimonSapin edited parent revisions, added: D10091: rhg: Add a `rhg.on-unsupported` configuration key; removed: D10092: rhg: Change default `rhg.on-unsupported` configuration to `abort`.Mar 4 2021, 6:10 AM

Please add a small description to your changeset that explains the new config value and why we would want rhg to host a fallback system itself when it already has a specific error code for this.

rust/rhg/src/main.rs
161	Not sure what warning you're planning to add
193–194	You've added this in the previous change, and are removing it now. Was this a `hg split` mistake?

Alphare requested changes to this revision.Mar 5 2021, 3:36 AM

This revision now requires changes to proceed.Mar 5 2021, 3:36 AM

In D10093#153624, @Alphare wrote:

Please add a small description to your changeset that explains the new config value and why we would want rhg to host a fallback system itself when it already has a specific error code for this.

+1. Having details in commit description will be nice.

SimonSapin marked 2 inline comments as done.Mar 9 2021, 4:39 AM

SimonSapin edited the summary of this revision. (Show Details)

SimonSapin updated this revision to Diff 26166.

I’ve expanded the commit message.

rust/rhg/src/main.rs
161	I forgot to remove this TODO comment when adding the "Blocking recursive fallback" message written to stderr above.
193–194	What happened is: This comment has been there for `Command::Abort` a while. When adding a second occurrence of this `let _ = ui.write_strerr(…);` pattern in the previous commit I copied the comment, but at the third in this commit this felt too redundant so I only kept one. I’ve amended the previous commit to only keep one occurrence.

Thanks for the thorough description.

pulkit accepted this revision.Mar 10 2021, 1:31 PM

This revision is now accepted and ready to land.Mar 10 2021, 1:31 PM

SimonSapin added a commit: rHG93e9f448273c: rhg: Add support for automatic fallback to Python.Mar 10 2021, 2:22 PM

Closed by commit rHG93e9f448273c: rhg: Add support for automatic fallback to Python (authored by SimonSapin). · Explain Why

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			rust/rhg/src/main.rs (65 lines)
M			tests/test-rhg.t (26 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	26027		Mar 2 2021, 2:58 PM	★	★
Diff 2	26028		Mar 2 2021, 4:21 PM	★	★
Diff 3	26166		Mar 9 2021, 4:39 AM	★	★
Diff 4	26230	rHG93e9f448273c5f716604a10c58ec899af2caf6e6	Mar 1 2021, 2:36 PM	★	★

Commit	Parents	Author	Summary	Date
ee14e0da3f0e	326dca9fedb9	Simon Sapin		Mar 1 2021, 2:36 PM

Status	Author	Revision
Closed	SimonSapin	D10095 tests: Add `--rhg` and `--with-rhg=<path>` options for `run-tests.py`
Abandoned	SimonSapin	D10094 rhg: Make fallback to Python the default behavior
Closed	SimonSapin	D10093 rhg: Add support for automatic fallback to Python
Abandoned	SimonSapin	D10092 rhg: Change default `rhg.on-unsupported` configuration to `abort`
Closed	SimonSapin	D10091 rhg: Add a `rhg.on-unsupported` configuration key
Closed	SimonSapin	D10090 rhg: Make configuration available as early as possible in main()

Diff 26027

rust/rhg/src/main.rs

	extern crate log;			extern crate log;
	use crate::ui::Ui;			use crate::ui::Ui;
	use clap::App;			use clap::App;
	use clap::AppSettings;			use clap::AppSettings;
	use clap::Arg;			use clap::Arg;
	use clap::ArgMatches;			use clap::ArgMatches;
	use format_bytes::format_bytes;			use format_bytes::format_bytes;
	use hg::config::Config;			use hg::config::Config;
	use hg::repo::{Repo, RepoError};			use hg::repo::{Repo, RepoError};
	use hg::utils::files::{get_bytes_from_os_str, get_path_from_bytes};			use hg::utils::files::{get_bytes_from_os_str, get_path_from_bytes};
	use hg::utils::SliceExt;			use hg::utils::SliceExt;
	use std::ffi::OsString;			use std::ffi::OsString;
	use std::path::PathBuf;			use std::path::PathBuf;
				use std::process::Command;

	mod blackbox;			mod blackbox;
	mod error;			mod error;
	mod exitcode;			mod exitcode;
	mod ui;			mod ui;
	use error::CommandError;			use error::CommandError;

	fn main_with_result(			fn main_with_result(
	Err(CommandError::UnsupportedFeature { .. }) => {			Err(CommandError::UnsupportedFeature { .. }) => {
	exitcode::UNIMPLEMENTED			exitcode::UNIMPLEMENTED
	}			}
	}			}
	}			}

	fn exit(			fn exit(
	ui: &Ui,			ui: &Ui,
	on_unsupported: OnUnsupported,			mut on_unsupported: OnUnsupported,
	result: Result<(), CommandError>,			result: Result<(), CommandError>,
	) -> ! {			) -> ! {
				if let (
				OnUnsupported::Fallback { executable },
				Err(CommandError::UnsupportedFeature { .. }),
				) = (&on_unsupported, &result)
				{
				let mut args = std::env::args_os();
				let executable_path = get_path_from_bytes(&executable);
				let this_executable = args.next().expect("exepcted argv[0] to exist");
				if executable_path == &PathBuf::from(this_executable) {
				// Avoid spawning infinitely many processes until resource
				// exhaustion.
				let _ = ui.write_stderr(&format_bytes!(
				b"Blocking recursive fallback. The 'rhg.fallback-executable = {}' config \
				points to `rhg` itself.\n",
				executable
				));
				// TODO: warn about this to explain `rhg.fallback-executable`
				AlphareUnsubmitted Done Not sure what warning you're planning to add Alphare: Not sure what warning you're planning to add
				SimonSapinAuthorUnsubmitted Done I forgot to remove this TODO comment when adding the "Blocking recursive fallback" message written to stderr above. SimonSapin: I forgot to remove this TODO comment when adding the "Blocking recursive fallback" message…
				// config
				on_unsupported = OnUnsupported::Abort
				} else {
				// `args` is now `argv[1..]` since we’ve already consumed `argv[0]`
				let result = Command::new(executable_path).args(args).status();
				match result {
				Ok(status) => std::process::exit(
				status.code().unwrap_or(exitcode::ABORT),
				),
				Err(error) => {
				let _ = ui.write_stderr(&format_bytes!(
				b"tried to fall back to a '{}' sub-process but got error {}\n",
				executable, format_bytes::Utf8(error)
				));
				on_unsupported = OnUnsupported::Abort
				}
				}
				}
				}
	match &result {			match &result {
	Ok(_) => {}			Ok(_) => {}
	Err(CommandError::Abort { message }) => {			Err(CommandError::Abort { message }) => {
	if !message.is_empty() {			if !message.is_empty() {
	// Ignore errors when writing to stderr, we’re already exiting			// Ignore errors when writing to stderr, we’re already exiting
	// with failure code so there’s not much more we can do.			// with failure code so there’s not much more we can do.
	let _ =			let _ =
	ui.write_stderr(&format_bytes!(b"abort: {}\n", message));			ui.write_stderr(&format_bytes!(b"abort: {}\n", message));
	}			}
	}			}
	Err(CommandError::UnsupportedFeature { message }) => {			Err(CommandError::UnsupportedFeature { message }) => {
	match on_unsupported {			match on_unsupported {
	OnUnsupported::Abort => {			OnUnsupported::Abort => {
	// Ignore errors when writing to stderr, we’re already
	// exiting with failure code so there’s
	// not much more we can do.
	let _ = ui.write_stderr(&format_bytes!(			let _ = ui.write_stderr(&format_bytes!(
				AlphareUnsubmitted Done You've added this in the previous change, and are removing it now. Was this a `hg split` mistake? Alphare: You've added this in the previous change, and are removing it now. Was this a `hg split`…
				SimonSapinAuthorUnsubmitted Done What happened is: This comment has been there for `Command::Abort` a while. When adding a second occurrence of this `let _ = ui.write_strerr(…);` pattern in the previous commit I copied the comment, but at the third in this commit this felt too redundant so I only kept one. I’ve amended the previous commit to only keep one occurrence. SimonSapin: What happened is: This comment has been there for `Command::Abort` a while. When adding a…
	b"unsupported feature: {}\n",			b"unsupported feature: {}\n",
	message			message
	));			));
	}			}
	OnUnsupported::AbortSilent => {}			OnUnsupported::AbortSilent => {}
				OnUnsupported::Fallback { .. } => unreachable!(),
	}			}
	}			}
	}			}
	std::process::exit(exit_code(&result))			std::process::exit(exit_code(&result))
	}			}

	macro_rules! subcommands {			macro_rules! subcommands {
	($( $command: ident )+) => {			($( $command: ident )+) => {
	///			///
	/// See `HgError::UnsupportedFeature` and `CommandError::UnsupportedFeature`.			/// See `HgError::UnsupportedFeature` and `CommandError::UnsupportedFeature`.
	enum OnUnsupported {			enum OnUnsupported {
	/// Print an error message describing what feature is not supported,			/// Print an error message describing what feature is not supported,
	/// and exit with code 252.			/// and exit with code 252.
	Abort,			Abort,
	/// Silently exit with code 252.			/// Silently exit with code 252.
	AbortSilent,			AbortSilent,
				/// Try running a Python implementation
				Fallback { executable: Vec<u8> },
	}			}

	impl OnUnsupported {			impl OnUnsupported {
				const DEFAULT: Self = OnUnsupported::Abort;
				const DEFAULT_FALLBACK_EXECUTABLE: &'static [u8] = b"hg";

	fn from_config(config: &Config) -> Self {			fn from_config(config: &Config) -> Self {
	let default = OnUnsupported::Abort;			match config
	match config.get(b"rhg", b"on-unsupported") {			.get(b"rhg", b"on-unsupported")
				.map(\|value\| value.to_ascii_lowercase())
				.as_deref()
				{
	Some(b"abort") => OnUnsupported::Abort,			Some(b"abort") => OnUnsupported::Abort,
	Some(b"abort-silent") => OnUnsupported::AbortSilent,			Some(b"abort-silent") => OnUnsupported::AbortSilent,
	None => default,			Some(b"fallback") => OnUnsupported::Fallback {
				executable: config
				.get(b"rhg", b"fallback-executable")
				.unwrap_or(Self::DEFAULT_FALLBACK_EXECUTABLE)
				.to_owned(),
				},
				None => Self::DEFAULT,
	Some(_) => {			Some(_) => {
	// TODO: warn about unknown config value			// TODO: warn about unknown config value
	default			Self::DEFAULT
	}			}
	}			}
	}			}
	}			}

tests/test-rhg.t

	#require rust			#require rust

	Define an rhg function that will only run if rhg exists			Define an rhg function that will only run if rhg exists
				$ export RHG="$RUNTESTDIR/../rust/target/release/rhg"
	$ rhg() {			$ rhg() {
	> if [ -f "$RUNTESTDIR/../rust/target/release/rhg" ]; then			> if [ -f "$RHG" ]; then
	> "$RUNTESTDIR/../rust/target/release/rhg" "$@"			> "$RHG" "$@"
	> else			> else
	> echo "skipped: Cannot find rhg. Try to run cargo build in rust/rhg."			> echo "skipped: Cannot find rhg. Try to run cargo build in rust/rhg."
	> exit 80			> exit 80
	> fi			> fi
	> }			> }

	Unimplemented command			Unimplemented command
	$ rhg unimplemented-command			$ rhg unimplemented-command
	$ rhg cat -r 0 original			$ rhg cat -r 0 original
	original content			original content
	Cat copied file should not display copy metadata			Cat copied file should not display copy metadata
	$ hg copy original copy_of_original			$ hg copy original copy_of_original
	$ hg commit -m "add copy of original"			$ hg commit -m "add copy of original"
	$ rhg cat -r 1 copy_of_original			$ rhg cat -r 1 copy_of_original
	original content			original content

				Fallback to Python
				$ rhg cat original
				unsupported feature: `rhg cat` without `--rev` / `-r`
				[252]
				$ export FALLBACK="--config rhg.on-unsupported=fallback"
				$ rhg cat original $FALLBACK
				original content

				$ rhg cat original $FALLBACK --config rhg.fallback-executable=false
				[1]

				$ rhg cat original $FALLBACK --config rhg.fallback-executable=hg-non-existent
				tried to fall back to a 'hg-non-existent' sub-process but got error $ENOENT$
				unsupported feature: `rhg cat` without `--rev` / `-r`
				[252]

				$ rhg cat original $FALLBACK --config rhg.fallback-executable="$RHG"
				Blocking recursive fallback. The 'rhg.fallback-executable = /home/simon/projects/hg/tests/../rust/target/release/rhg' config points to `rhg` itself.
				unsupported feature: `rhg cat` without `--rev` / `-r`
				[252]

	Requirements			Requirements
	$ rhg debugrequirements			$ rhg debugrequirements
	dotencode			dotencode
	fncache			fncache
	generaldelta			generaldelta
	revlogv1			revlogv1
	sparserevlog			sparserevlog
	store			store