This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1581

rust: implementation of `hg`
ClosedPublic

Authored by indygreg on Dec 4 2017, 1:59 AM.

Details

Reviewers
yuja
durin42
Group Reviewers
hg-reviewers
Commits
rHG964212780daf: rust: implementation of `hg`
Summary

This commit provides a mostly-working implementation of the
hg script in Rust along with scaffolding to support Rust in
the repository.

If you are familiar with Rust, the contents of the added rust/
directory should be pretty straightforward. We create an "hgcli"
package that implements a binary application to run Mercurial.
The output of this package is an "hg" binary.

Our Rust hg (henceforth "rhg") essentially is a port of the existing
hg Python script. The main difference is the creation of the embedded
CPython interpreter is handled by the binary itself instead of relying
on the shebang. In that sense, rhg is more similar to the "exe wrapper"
we currently use on Windows. However, unlike the exe wrapper, rhg does
not call the hg Python script. Instead, it uses the CPython APIs to
import mercurial modules and call appropriate functions. The amount of
code here is surprisingly small.

It is my intent to replace the existing C-based exe wrapper with rhg.
Preferably in the next Mercurial release. This should be achievable -
at least for some Mercurial distributions. The future/timeline for
rhg on other platforms is less clear. We already ship a hg.exe on
Windows. So if we get the quirks with Rust worked out, shipping a
Rust-based hg.exe should hopefully not be too contentious.

Now onto the implementation.

We're using python27-sys and the cpython crates for talking to the
CPython API. We currently don't use too much functionality of the
cpython crate and could have probably cut it out. However, it does
provide a reasonable abstraction over unsafe {} CPython function
calls. While we still have our fair share of those, at least we're
not dealing with too much refcounting, error checking, etc. So I
think the use of the cpython crate is justified. Plus, there is
not-yet-implemented functionality that could benefit from cpython. I
see our use of this crate only increasing.

The cpython and python27-sys crates are not without their issues.
The cpython crate didn't seem to account for the embedding use case
in its design. Instead, it seems to assume that you are building
a Python extension. It is making some questionable decisions around
certain CPython APIs. For example, it insists that
PyEval_ThreadsInitialized() is called and that the Python code
likely isn't the main thread in the underlying application. It
is also missing some functionality that is important for embedded
use cases (such as exporting the path to the Python interpreter
from its build script). After spending several hours trying to
wrangle python27-sys and cpython, I gave up and forked the project
on GitHub. Our Cargo.toml tracks this fork. I'm optimistic that
the upstream project will accept our contributions and we can
eventually unfork.

There is a non-trivial amount of code in our custom Cargo build
script. Our build.rs (which is called as part of building the hgcli
crate):

  • Validates that the Python interpreter that was detected by the python27-sys crate provides a shared library (we only support shared library linking at this time - although this restriction could be loosened).
  • Validates that the Python is built with UCS-4 support. This ensures maximum Unicode compatibility.
  • Exports variables to the crate build allowing the built crate to e.g. find the path to the Python interpreter.

The produced rhg should be considered alpha quality. There are several
known deficiencies. Many of these are documented with inline TODOs.

Probably the biggest limitation of rhg is that it assumes it is
running from the ./rust/target/<target> directory of a source
distribution. So, rhg is currently not very practical for real-world
use. But, if you can cargo build it, running the binary *should*
yield a working Mercurial CLI.

In order to support using rhg with the test harness, we needed to hack
up run-tests.py so the path to Mercurial's Python files is set properly.
The change is extremely hacky and is only intended to be a stop-gap
until the test harness gains first-class support for installing rhg.
This will likely occur after we support running rhg outside the
source directory.

Despite its officially alpha quality, rhg copes extremely well with
the test harness (at least on Linux). Using
run-tests.py --with-hg ../rust/target/debug/hg, I only encounter
the following failures:

  • test-run-tests.t -- Warnings emitted about using an unexpected Mercurial library. This is due to the hacky nature of setting the Python directory when run-tests.py detected rhg.
  • test-devel-warnings.t -- Expected stack trace missing frame for hg (This is expected since we no longer have an hg script!)
  • test-convert.t -- Test running $PYTHON "$BINDIR"/hg, which obviously assumes hg is a Python script.
  • test-merge-tools.t -- Same assumption about hg being executable with Python.
  • test-http-bad-server.t -- Seeing exit code 255 instead of 1 around line 358.
  • test-blackbox.t -- Exit code 255 instead of 1.
  • test-basic.t -- Exit code 255 instead of 1.

It certainly looks like we have a bug around exit code handling. I
don't think it is severe enough to hold up review and landing of this
initial implementation. Perfect is the enemy of good.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Dec 4 2017, 1:59 AM
dlax added a subscriber: dlax.Dec 4 2017, 4:12 AM
durin42 added a subscriber: durin42.Dec 4 2017, 5:28 PM
durin42 added inline comments.
rust/hgcli/src/main.rs
131

You might be able to usefully avoid all the unwrap() in here by having an extra layer, eg

fn run() -> Result<(), failure::Error> {
...

let program_name = CString::new(env.python_exe.to_str()?)?.as_ptr();

}

and then

fn main() {

let mut exit_code;
match run() {
  Ok(_) => exit_code = 0,
  Err(e) => { println!("{:?}", e); exit_code = 1; },

}

std::process::exit(exit_code);

}

etc

quark added a subscriber: quark.Dec 4 2017, 6:12 PM
quark added inline comments.
rust/hgcli/src/main.rs
20–32

Maybe use extern crate python27_sys so part of them get defined.

35–38

Have you tried rustfmt? It will remove the space before :.

112

Maybe use .expect("setdefaultencoding failed").

116

Maybe py: Python. That's what rust-cpython uses. It implements Copy.

121

Probably use PyResult<int> since dispatch.run returns the exit code.

173

255 is probably better since that's an uncaught exception.

indygreg updated this revision to Diff 4112.Dec 5 2017, 2:44 AM
yuja added a subscriber: yuja.Dec 5 2017, 9:40 AM
yuja added inline comments.
rust/hgcli/src/main.rs
101

Perhaps we'll need a utility function for platform-specific cstr
conversion.

On Unix, the story is simple. We can use OsStrExt. On Windows,
maybe we'll have to first convert it to "wide characters" by OsStrExt
and then call WideCharToMultiByte to convert it back to ANSI bytes sequence.

I have no idea if Rust stdlib provides a proper way to convert
OsStr to Windows ANSI bytes.

https://stackoverflow.com/a/38948854
https://msdn.microsoft.com/en-us/library/windows/desktop/dd374130(v=vs.85).aspx

indygreg edited the summary of this revision. (Show Details)Dec 7 2017, 2:54 AM
indygreg updated this revision to Diff 4165.
indygreg edited the summary of this revision. (Show Details)Dec 8 2017, 2:21 AM
indygreg retitled this revision from [RFC] rust: Rust implementation of `hg` and standalone packaging to rust: implementation of `hg`.
indygreg updated this revision to Diff 4241.
indygreg updated this revision to Diff 4244.Dec 8 2017, 2:26 AM
indygreg marked 6 inline comments as done.Dec 8 2017, 2:32 AM

I removed the standalone distribution code and cleaned things up a bit.

Over 97% of the tests pass with the Rust hg on Linux. And the failures seem reasonable.

There's a bit of work to be done around packaging, Windows support, Rust linting, etc. But I'd like to get something landed so others have something to play with and so we're not reviewing a massive patch.

What I'm trying to say is "I think this is ready for a real review."

rust/hgcli/src/main.rs
101

I documented a potential path forward in the code. I was thinking we can address this is a follow-up because it is non-trivial to implement. And we may want to do something funky, such as inject the raw bytes into mercurial.argv or something. Since we can call the Windows API directly, we can actually do that now. This opens up a lot of possibilities for encoding...

121

`dispatch.run() actually calls sys.exit()`. There is a bug around somewhere in this code around exit code handling. But it only presents in a few tests, which is odd. I documented that in the commit message.

indygreg updated this revision to Diff 4525.Dec 17 2017, 1:31 PM
yuja requested changes to this revision.Jan 5 2018, 2:38 AM

Suppose this is a kind of contrib code, I think it's good enough to accept.
Can you drop Cargo.lock file?

rust/Cargo.lock
2

Perhaps Cargo.lock should be excluded from the commit.

rust/hgcli/build.rs
89

Nit: const ?

rust/hgcli/src/main.rs
63

Nit: probably we don't have to clone() here, just move these values.

This revision now requires changes to proceed.Jan 5 2018, 2:38 AM
indygreg updated this revision to Diff 4744.Jan 9 2018, 9:07 PM
indygreg added inline comments.Jan 9 2018, 9:07 PM
rust/Cargo.lock
2

According to https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries (and other places I found on the Internet), Cargo.lock files under version control are a good practice for binaries - but not libraries.

Since this Cargo.lock encompasses the hgcli binary, I think that counts.

FWIW, Firefox vendors Cargo.lock files for libraries. Although those libraries are attached to C++ binaries, so there isn't an appropriate Rust binary to attach to. But a main purpose of Cargo.lock files is to add determinism. For that reason, I think we want them vendored.

indygreg marked an inline comment as done.Jan 9 2018, 9:10 PM
indygreg updated this revision to Diff 4745.
indygreg marked an inline comment as done.Jan 9 2018, 9:10 PM
yuja added inline comments.Jan 10 2018, 8:06 AM
rust/Cargo.lock
2

Cargo.lock files under version control are a good practice for binaries

Okay, I didn't know that, thanks. (FWIW, I don't like the idea of version-controlling
strict build dependencies, but let's just follow their rule.)

Can we move Cargo.lock under hgcli directory, then? Sounds like we'll need a
separate Cargo.lock file for "libraries".

indygreg updated this revision to Diff 4753.Jan 10 2018, 11:53 AM
indygreg marked 3 inline comments as done.Jan 10 2018, 11:53 AM
cramertj added a subscriber: cramertj.Jan 10 2018, 2:31 PM

Looks pretty good to me-- left a couple nits inline.

There are a lot of uses of unwrap, expect, and panic that can (and probably should) be replaced with proper error handling using Result (and the failure crate).

There are also a couple of crates that provide safe bindings to Python interpreters-- I'm not sure what your external dependency situation is, but you might consider using something like https://crates.io/crates/pyo3 rather than writing your own unsafe calls to the python interpreter.

rust/hgcli/build.rs
13

Nit: if you move this import into have_shared, you'll only need one cfg and it'll be easier to validate that the proper deps are available for the proper platforms.

89

Nit: not sure what version you're targeting, but 'static is automatic for const vars, so you could write [&str; 2]

rust/hgcli/src/main.rs
46

Nit: you can just write &str. Also, I'm not familiar with what you're trying to do here, but is the PYTHON_INTERPRETER always determined at compile-time? It seems like something you might want to switch on at runtime. Is that not the case?

126

If it needs to live for the whole time, consider storing it in a static or similar. There's a subtle unsafety here: if this program panics (due to panic, unwrap, or expect, program_name will be dropped (free'd) before the python interpreter is killed (when the process ends, that is-- Finalize won't ever be called in that case). I don't know how much of an issue this will be in practice, but it's something to think about.

indygreg added a comment.Jan 10 2018, 4:46 PM
In D1581#31029, @cramertj wrote:

There are a lot of uses of unwrap, expect, and panic that can (and probably should) be replaced with proper error handling using Result (and the failure crate).

I definitely agree. I still consider this just beyond proof-of-concept code. We'll definitely want to shore things up before we ship. Perfect is very much the enemy of good at this stage.

There are also a couple of crates that provide safe bindings to Python interpreters-- I'm not sure what your external dependency situation is, but you might consider using something like https://crates.io/crates/pyo3 rather than writing your own unsafe calls to the python interpreter.

pyo3 requires non-stable Rust features last I checked. That makes it a non-starter for us at this time (since downstream packagers will insist on only using stable Rust).

If other external dependencies provide the interfaces we need, I'm open to taking those dependencies. But this crate is focused on embedding a Python interpreter. Most (all?) of the Rust+Python crates I found seemed to target the "implementing Python extensions with Rust" use case, not embedding Python. As such, their embedding API support is very lacking. I even had to fork rust-cpython because it didn't implement the proper APIs and is forcing its extension-centric religion on consumers. I've upstreamed most of my modifications though. So hopefully the fork doesn't live much longer...

cramertj added a comment.Jan 10 2018, 4:49 PM
In D1581#31035, @indygreg wrote:
In D1581#31029, @cramertj wrote:

There are a lot of uses of unwrap, expect, and panic that can (and probably should) be replaced with proper error handling using Result (and the failure crate).

I definitely agree. I still consider this just beyond proof-of-concept code. We'll definitely want to shore things up before we ship. Perfect is very much the enemy of good at this stage.

There are also a couple of crates that provide safe bindings to Python interpreters-- I'm not sure what your external dependency situation is, but you might consider using something like https://crates.io/crates/pyo3 rather than writing your own unsafe calls to the python interpreter.

pyo3 requires non-stable Rust features last I checked. That makes it a non-starter for us at this time (since downstream packagers will insist on only using stable Rust).
If other external dependencies provide the interfaces we need, I'm open to taking those dependencies. But this crate is focused on embedding a Python interpreter. Most (all?) of the Rust+Python crates I found seemed to target the "implementing Python extensions with Rust" use case, not embedding Python. As such, their embedding API support is very lacking. I even had to fork rust-cpython because it didn't implement the proper APIs and is forcing its extension-centric religion on consumers. I've upstreamed most of my modifications though. So hopefully the fork doesn't live much longer...

SGTM! Thanks for clarifying.

durin42 accepted this revision.Jan 10 2018, 5:30 PM
Closed by commit rHG964212780daf: rust: implementation of `hg` (authored by indygreg). · Explain WhyJan 10 2018, 6:02 PM
This revision was automatically updated to reflect the committed changes.
indygreg marked 7 inline comments as done.Jan 10 2018, 10:41 PM

I'll address this and other review feedback in follow-up patches.

Thanks for the review!

rust/hgcli/src/main.rs
46

This is meant to be dynamic. The gist of this code is we're trying to find the location of the Python install given various search strategies. The search strategy is (currently) defined at compile time. And this localdev search strategy defines the path to Python at compile time. Look for my features and documentation for this code later. (I stripped out unused code from this patch to make it smaller.)

indygreg marked 3 inline comments as done.Jan 10 2018, 10:41 PM
kevincox added a subscriber: kevincox.Jan 11 2018, 6:01 AM

Overall it looks great. I added a bunch of nitpicks. The most common suggestion was to add some contextual information to error messages. Other then that mostly minor style improvements. Feel free to push back on anything you don't agree with :)

rust/hgcli/build.rs
34
assert!(
    Path::new(&python).exists(),
    "Python interpreter {} does not exist; this should never happen",
    python);

I would also recommend {:?} as the quotes highlight the injected variable nicely and it will make some hard-to-notice things more visible due to escaping.

89

I would also recommend using a slice if you don't intend the size of the array to be part of the type signature.

const REQUIRED_CONFIG_FLAGS: &[&str] = &["Py_USING_UNICODE", "WITH_THREAD"];
111

Use assert!.

117

Use assert!

128
#[cfg(not(target_os = "windows"))]
assert_eq!(
    config.config.get("Py_UNICODE_SIZE"), Some("4"),
     "Detected Python doesn't support UCS-4 code points");
rust/hgcli/src/main.rs
38

Use expect for a better error message. .expect("Error getting executable path")

92

This method allows paths that aren't valid UTF-8 by avoiding ever becoming a str.

CString::new(env.python_home.as_ref().as_bytes())

I would also change the unwrap to .expect("Error setting python home").

100

It appears that HG accepts HGUNICODEPEDANTRY= as not enabling unicode pedantry. Maybe the behavior should be the same here. Untested code below should work.

let pedantry = !env::var("HGUNICODEPEDANTRY").unwrap_or("").is_empty();
112

.expect("Error accessing sys.path")

134

CString::new(env.python_exe.as_ref().as_bytes())

135

.expect("Error setting program name")

201

Since this method returns a Result why not handle the error?

let sys_mod = py.import("sys")?;
yuja added inline comments.Jan 11 2018, 7:31 AM
rust/hgcli/src/main.rs
134

That's more correct on Unix, but wouldn't work on Windows since
native string is UTF-16 variant (Rust) or ANSI (Python 2.7).

kevincox added inline comments.Jan 11 2018, 8:09 AM
rust/hgcli/src/main.rs
134

Oops, for some reason I thought this was in a unix block. as_bytes() isn't actually available otherise. I think that this is fine then. non-UTF-8 paths shouldn't be a major issue..

ruuda added a subscriber: ruuda.Jan 11 2018, 12:09 PM

Awesome! I have just a few things that could be written more briefly.

rust/hgcli/build.rs
40

You can safely indent those, leading whitespace on the continuation line will be stripped from the string literal.

108

There is no need to match:

let result = config.config.get(*key) == Some("1");

Or using assert as @kevincox recommends:

assert_eq!(config.config.get(*key), Some("1"), "Detected ...");
rust/hgcli/src/main.rs
188

There exists Result::map_err for this:

result = run_py(&env, py).map_err(|err| {
    err.print(py);
    255
});

Revision Contents
Changeset List

PathPackages
M.hgignore (2 lines)
AMcontrib/STANDALONE-MERCURIAL.rst (70 lines)
AMcontrib/build-standalone.py (390 lines)
AMrust/.hgignore (1 line)
AMrust/.cargo/config (7 lines)
AMrust/Cargo.lock (136 lines)
AMrust/Cargo.toml (2 lines)
AMrust/README.rst (32 lines)
AMrust/hgcli/Cargo.toml (24 lines)
AMrust/hgcli/build.rs (106 lines)
AMrust/hgcli/src/main.rs (200 lines)

Diff 4112

.hgignore

syntax: regexp syntax: regexp
^\.pc/ ^\.pc/
^\.(pydev)?project ^\.(pydev)?project
# hackable windows distribution additions # hackable windows distribution additions
^hg-python ^hg-python
^hg.py$ ^hg.py$
subinclude:rust/.hgignore

contrib/STANDALONE-MERCURIAL.rst

  • This file was added.
====================
Standalone Mercurial
====================
*Standalone Mercurial* is a generic term given to a distribution
of Mercurial that is standalone and has minimal dependencies on
the host (typically just the C runtime library). Instead, most of
Mercurial's dependencies are included in the distribution. This
includes a Python interpreter.
Architecture
============
A standalone Mercurial distribution essentially consists of the
following elements:
* An `hg` binary executable
* A Python interpreter shared library
* The Python standard library
* 3rd party Python packages to enhance the Mercurial experience
* Mercurial's Python packages
* Mercurial support files (help content, default config files, etc)
* Any additional support files (e.g. shared library dependencies)
From a high-level, the `hg` binary has a shared library dependency
on `libpython`. The binary is configured to load the `libpython`
that ships with the Mercurial distribution. When started, the
`hg` binary assesses its state, configures an embedded Python
interpreter, and essentially invoke Mercurial's `main()` function.
Build Requirements
==================
Universal
---------
* Python 2.7 (to run the build script)
* A working Rust and Cargo installation
Linux
-----
* Dependencies to build Python 2.7 from source (GNU make, autoconf,
various dependencies for extensions)
* The `patchelf` tool
MacOS
-----
* Xcode
Windows
-------
* Microsoft Visual C+ Compiler for Python 2.7 (https://www.microsoft.com/en-us/download/details.aspx?id=44266)
Building
========
To build standalone Mercurial, run the following::
$ python2.7 contrib/build-standalone.py
This will:
1. Obtain a Python distribution (either by compiling from source
or downloading a pre-built distribution)
2. Build Mercurial Rust components
3. Build Mercurial Python components
4. Produce an *archive* suitable for distribution

contrib/build-standalone.py

  • This file was added.
PropertyOld ValueNew Value
File Modenull100755
#!/usr/bin/env python2.7
# build-standalone.py - Create a standalone distribution of Mercurial.
#
# Copyright 2017 Gregory Szorc <gregory.szorc@gmail.com>
#
# This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version.
"""Create a standalone Mercurial distribution.
This script does the bulk of the work for creating a standalone Mercurial
distribution.
"""
import errno
import gzip
import hashlib
import io
import multiprocessing
import os
import shutil
import stat
import subprocess
import sys
import tarfile
import tempfile
import urllib2
try:
import lzma
except ImportError:
lzma = None
PYTHON_ARCHIVES = {
'version': '2.7.14',
'url': 'https://www.python.org/ftp/python/{version}/{prefix}-{version}.{suffix}',
'gz': {
'sha256': '304c9b202ea6fbd0a4a8e0ad3733715fbd4749f2204a9173a58ec53c32ea73e8',
'prefix': 'Python',
'suffix': 'tgz',
'tar_mode': 'r:gz',
},
'xz': {
'sha256': '71ffb26e09e78650e424929b2b457b9c912ac216576e6bd9e7d204ed03296a66',
'prefix': 'Python',
'suffix': 'xz',
'tar_mode': 'r:xz',
},
'msi32': {
'sha256': '450bde0540341d4f7a6ad2bb66639fd3fac1c53087e9844dc34ddf88057a17ca',
'prefix': 'python',
'suffix': 'msi',
},
'msi64': {
'sha256': 'af293df7728b861648162ba0cd4a067299385cb6a3f172569205ac0b33190693',
'prefix': 'python',
'suffix': 'amd64.msi',
}
}
def hash_file(fh):
hasher = hashlib.sha256()
while True:
chunk = fh.read(16384)
if not chunk:
break
hasher.update(chunk)
return hasher.hexdigest()
def makedirs(path):
try:
os.makedirs(path)
except OSError as e:
if e.errno != errno.EEXIST:
raise
def _ensure_python_source(dest_dir):
"""Ensure the Python source code is extracted to a path."""
makedirs(dest_dir)
if lzma:
archive = PYTHON_ARCHIVES['xz']
else:
archive = PYTHON_ARCHIVES['gz']
archive_path = os.path.join(dest_dir,
'python-%s.%s' % (PYTHON_ARCHIVES['version'],
archive['suffix']))
if os.path.exists(archive_path):
with open(archive_path, 'rb') as fh:
if hash_file(fh) != archive['sha256']:
print('%s has unexpected hash; removing' % archive_path)
os.unlink(archive_path)
if not os.path.exists(archive_path):
url = PYTHON_ARCHIVES['url'].format(
version=PYTHON_ARCHIVES['version'],
prefix=archive['prefix'],
suffix=archive['suffix'])
print('downloading %s' % url)
req = urllib2.urlopen(url)
if req.getcode() != 200:
raise Exception('non-200 HTTP response downloading Python: %d' % req.getcode())
buf = io.BytesIO()
while True:
chunk = req.read(16384)
if not chunk:
break
buf.write(chunk)
buf.seek(0)
if hash_file(buf) != archive['sha256']:
raise Exception('Python hash mismatch')
buf.seek(0)
with open(archive_path, 'wb') as fh:
fh.write(buf.getvalue())
# Assume if a single file from the archive is present that we don't need
# to re-extract.
if os.path.exists(os.path.join(dest_dir, 'configure')):
print('extracted python source code found; using without modifications')
return
print('extracting %s to %s' % (archive_path, dest_dir))
with tarfile.open(archive_path, archive['tar_mode']) as tf:
prefix = 'Python-%s' % PYTHON_ARCHIVES['version']
for ti in tf:
assert ti.name.startswith(prefix)
ti.name = ti.name[len(prefix):].lstrip('/')
tf.extract(ti, dest_dir)
def _build_python(state):
source_dir = state['python_source_dir']
build_dir = state['python_build_dir']
_ensure_python_source(source_dir)
makedirs(build_dir)
# TODO use a more sensible filesystem layout for Python in cases
# where the files will be installed alongside other system files
# (e.g. when producing deb or rpm archives).
if not os.path.exists(os.path.join(build_dir, 'config.status')):
subprocess.check_call([
os.path.join(source_dir, 'configure'),
'--prefix', '/hgpython',
'--enable-shared',
'--enable-unicode=ucs4',
# TODO enable optimizations
# '--enable-optimizations',
# '--enable-lto',
], cwd=build_dir)
subprocess.check_call([
'make', '-j%d' % multiprocessing.cpu_count(),
], cwd=build_dir)
def install_python(state):
"""Installs Python in the standalone directory.
Python is installed to the `hgpython/` sub-directory. The layout of
this directory resembles a typical Python distribution. In fact, the
Python installation could be used on its own, just like any other
Python installation.
"""
# TODO on Windows, obtain Python files from official, self-contained
# binary distribution (via an MSI).
_build_python(state)
build_dir = state['python_build_dir']
py_dir = state['python_install_dir']
if os.path.exists(os.path.join(py_dir, 'bin', 'python')):
print('python already installed in %s; skipping `make install`' %
py_dir)
else:
subprocess.check_call([
'make',
'-j%d' % multiprocessing.cpu_count(),
'install',
'DESTDIR=%s' % state['install_dir'],
], cwd=build_dir)
# Update shared library references to be relative to binary.
# TODO compile Python in such a way that this isn't necessary.
if sys.platform.startswith('linux'):
subprocess.check_call([
'patchelf',
'--set-rpath',
'$ORIGIN/../lib',
state['python_bin'],
])
elif sys.platform == 'darwin':
subprocess.check_call([
'install_name_tool', '-change',
'/hgpython/lib/libpython2.7.dylib',
'@loader_path/../lib/libpython2.7.dylib',
state['python_bin'],
])
def install_rust_components(state):
rust_dir = os.path.join(state['root_dir'], 'rust', 'hgcli')
env = dict(os.environ)
# Tell cpython's build.rs to use our Python binary.
env['PYTHON_SYS_EXECUTABLE'] = os.path.join(
state['python_install_dir'], 'bin', 'python2.7')
# Tell our build.rs where to find libpython.
env['HG_STANDALONE_LINK_PATH'] = os.path.join(
state['python_install_dir'], 'lib')
subprocess.check_call(['cargo', 'build', '--release', '-v'],
cwd=rust_dir, env=env)
subprocess.check_call([
'cargo',
'install',
'--force',
'--root', state['install_dir'],
], cwd=rust_dir, env=env)
# TODO figure out how to link properly via Cargo.
# Adjust rpath so libpython is loaded from a relative path.
if sys.platform.startswith('linux'):
subprocess.check_call([
'patchelf',
'--set-rpath',
'$ORIGIN/../hgpython/lib',
state['hg_bin'],
])
elif sys.platform == 'darwin':
subprocess.check_call([
'install_name_tool', '-change',
'/System/Library/Frameworks/Python.framework/Versions/2.7/Python',
'@loader_path/../lib/libpython2.7.dylib',
state['hg_bin'],
])
def install_mercurial(state):
"""Install Mercurial files into the distribution."""
install_dir = os.path.join(state['install_dir'])
python = os.path.join(state['python_install_dir'], 'bin', 'python')
temp_dir = tempfile.mkdtemp(dir=state['build_dir'])
try:
subprocess.check_call([
python, 'setup.py',
'build',
'install',
# These are the only files we care about.
'--install-lib', os.path.join(install_dir, 'mercurial'),
'--install-data', os.path.join(temp_dir, 'data'),
'--install-headers', os.path.join(temp_dir, 'headers'),
'--install-platlib', os.path.join(temp_dir, 'platlib'),
'--install-purelib', os.path.join(temp_dir, 'purelib'),
# `hg` is replaced by our binary version.
'--install-scripts', os.path.join(temp_dir, 'bin'),
],
cwd=state['root_dir'])
finally:
temp_files = set()
for root, dirs, files in os.walk(temp_dir):
for f in files:
full = os.path.join(root, f)
temp_files.add(full[len(temp_dir)+1:])
shutil.rmtree(temp_dir)
expected = {
'bin/hg',
}
extra = temp_files - expected
if extra:
raise Exception('unknown extra files were installed: %s' %
', '.join(sorted(extra)))
def _run_hg(args):
env = dict(os.environ)
env['HGPLAIN'] = '1'
env['HGRCPATH'] = ''
with open(os.devnull, 'wb') as devnull:
return subprocess.check_output([state['hg_bin']] + args,
env=env,
stderr=devnull)
def verify_hg(state):
print('running `hg version`')
try:
print(_run_hg(['version']))
except subprocess.CalledProcessError as e:
print('error invoking `hg version`')
print(e.output)
sys.exit(1)
def get_revision_info(state):
res = _run_hg(['-R', state['root_dir'], 'log', '-r', '.', '-T', '{node} {date}'])
node, date = res.split(' ')
return node, int(float(date))
def _get_archive_files(state):
# Ideally we wouldn't have any ignores.
IGNORE = {
'.crates.toml',
}
for root, dirs, files in os.walk(state['install_dir']):
# sorts are here for determinism.
dirs.sort()
for f in sorted(files):
full = os.path.join(root, f)
rel = full[len(state['install_dir']) + 1:]
if rel in IGNORE:
continue
yield full, rel
def create_tar(state, ts):
print('writing %s' % state['tar_path'])
with tarfile.TarFile(state['tar_path'], 'w') as tf:
for full, rel in _get_archive_files(state):
with open(full, 'rb') as fh:
ti = tf.gettarinfo(full, rel)
if ti.mode & (stat.S_ISUID | stat.S_ISGID):
print('setuid or setgid bits set: %s' % full)
# Normalize mtime to commit time.
ti.mtime = ts
# Normalize uid/gid to root:root.
ti.uid = 0
ti.gid = 0
ti.uname = ''
ti.gname = ''
tf.addfile(ti, fh)
#gz = state['tar_path'] + '.gz'
#print('writing %s' % gz)
#with open(state['tar_path'], 'rb') as ifh, gzip.GzipFile(gz, 'wb') as ofh:
# shutil.copyfileobj(ifh, ofh)
if __name__ == '__main__':
root = os.path.normpath(os.path.join(os.path.dirname(__file__), '..'))
root = os.path.abspath(root)
build_dir = os.path.join(root, 'build')
python_install_dir = os.path.join(build_dir, 'standalone', 'hgpython')
state = {
'root_dir': root,
'build_dir': build_dir,
'install_dir': os.path.join(build_dir, 'standalone'),
'python_source_dir': os.path.join(build_dir, 'python-src'),
'python_build_dir': os.path.join(build_dir, 'python-build'),
'python_install_dir': python_install_dir,
'python_bin': os.path.join(python_install_dir, 'bin', 'python2.7'),
'hg_bin': os.path.join(build_dir, 'standalone', 'bin', 'hg'),
'tar_path': os.path.join(build_dir, 'standalone.tar'),
}
makedirs(state['install_dir'])
install_python(state)
install_rust_components(state)
install_mercurial(state)
verify_hg(state)
node, ts = get_revision_info(state)
create_tar(state, ts)

rust/.hgignore

  • This file was added.
target/

rust/.cargo/config

  • This file was added.
# Rust builds with a modern MSVC and uses a newer CRT.
# Python 2.7 has a shared library dependency on an older CRT (msvcr90.dll).
# We statically link the modern CRT to avoid multiple msvcr*.dll libraries
# being loaded and Python possibly picking up symbols from the newer runtime
# (which would be loaded first).
[target.'cfg(target_os = "windows")']
rustflags = ["-Ctarget-feature=+crt-static"]

rust/Cargo.lock

  • This file was added.
[[package]]
name = "aho-corasick"
yujaUnsubmitted
Done

Perhaps Cargo.lock should be excluded from the commit.

yuja: Perhaps Cargo.lock should be excluded from the commit.
indygregAuthorUnsubmitted
Done

According to https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries (and other places I found on the Internet), Cargo.lock files under version control are a good practice for binaries - but not libraries.

Since this Cargo.lock encompasses the hgcli binary, I think that counts.

FWIW, Firefox vendors Cargo.lock files for libraries. Although those libraries are attached to C++ binaries, so there isn't an appropriate Rust binary to attach to. But a main purpose of Cargo.lock files is to add determinism. For that reason, I think we want them vendored.

indygreg: According to https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version…
yujaUnsubmitted
Done

Cargo.lock files under version control are a good practice for binaries

Okay, I didn't know that, thanks. (FWIW, I don't like the idea of version-controlling
strict build dependencies, but let's just follow their rule.)

Can we move Cargo.lock under hgcli directory, then? Sounds like we'll need a
separate Cargo.lock file for "libraries".

yuja: > Cargo.lock files under version control are a good practice for binaries Okay, I didn't know…
version = "0.5.3"
source = "registry+https://github.com/rust-lang/crates.io-index"
dependencies = [
"memchr 0.1.11 (registry+https://github.com/rust-lang/crates.io-index)",
]
[[package]]
name = "cpython"
version = "0.1.0"
source = "git+https://github.com/dgrunwald/rust-cpython.git?rev=b35031e2670d7571f03c313cde8fd91105bd5322#b35031e2670d7571f03c313cde8fd91105bd5322"
dependencies = [
"libc 0.2.34 (registry+https://github.com/rust-lang/crates.io-index)",
"num-traits 0.1.41 (registry+https://github.com/rust-lang/crates.io-index)",
"python27-sys 0.1.2 (git+https://github.com/dgrunwald/rust-cpython.git?rev=b35031e2670d7571f03c313cde8fd91105bd5322)",
]
[[package]]
name = "hgcli"
version = "0.1.0"
dependencies = [
"cpython 0.1.0 (git+https://github.com/dgrunwald/rust-cpython.git?rev=b35031e2670d7571f03c313cde8fd91105bd5322)",
"libc 0.2.34 (registry+https://github.com/rust-lang/crates.io-index)",
"which 1.0.3 (registry+https://github.com/rust-lang/crates.io-index)",
]
[[package]]
name = "kernel32-sys"
version = "0.2.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
dependencies = [
"winapi 0.2.8 (registry+https://github.com/rust-lang/crates.io-index)",
"winapi-build 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)",
]
[[package]]
name = "libc"
version = "0.2.34"
source = "registry+https://github.com/rust-lang/crates.io-index"
[[package]]
name = "memchr"
version = "0.1.11"
source = "registry+https://github.com/rust-lang/crates.io-index"
dependencies = [
"libc 0.2.34 (registry+https://github.com/rust-lang/crates.io-index)",
]
[[package]]
name = "num-traits"
version = "0.1.41"
source = "registry+https://github.com/rust-lang/crates.io-index"
[[package]]
name = "python27-sys"
version = "0.1.2"
source = "git+https://github.com/dgrunwald/rust-cpython.git?rev=b35031e2670d7571f03c313cde8fd91105bd5322#b35031e2670d7571f03c313cde8fd91105bd5322"
dependencies = [
"libc 0.2.34 (registry+https://github.com/rust-lang/crates.io-index)",
"regex 0.1.80 (registry+https://github.com/rust-lang/crates.io-index)",
]
[[package]]
name = "regex"
version = "0.1.80"
source = "registry+https://github.com/rust-lang/crates.io-index"
dependencies = [
"aho-corasick 0.5.3 (registry+https://github.com/rust-lang/crates.io-index)",
"memchr 0.1.11 (registry+https://github.com/rust-lang/crates.io-index)",
"regex-syntax 0.3.9 (registry+https://github.com/rust-lang/crates.io-index)",
"thread_local 0.2.7 (registry+https://github.com/rust-lang/crates.io-index)",
"utf8-ranges 0.1.3 (registry+https://github.com/rust-lang/crates.io-index)",
]
[[package]]
name = "regex-syntax"
version = "0.3.9"
source = "registry+https://github.com/rust-lang/crates.io-index"
[[package]]
name = "thread-id"
version = "2.0.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
dependencies = [
"kernel32-sys 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)",
"libc 0.2.34 (registry+https://github.com/rust-lang/crates.io-index)",
]
[[package]]
name = "thread_local"
version = "0.2.7"
source = "registry+https://github.com/rust-lang/crates.io-index"
dependencies = [
"thread-id 2.0.0 (registry+https://github.com/rust-lang/crates.io-index)",
]
[[package]]
name = "utf8-ranges"
version = "0.1.3"
source = "registry+https://github.com/rust-lang/crates.io-index"
[[package]]
name = "which"
version = "1.0.3"
source = "registry+https://github.com/rust-lang/crates.io-index"
dependencies = [
"libc 0.2.34 (registry+https://github.com/rust-lang/crates.io-index)",
]
[[package]]
name = "winapi"
version = "0.2.8"
source = "registry+https://github.com/rust-lang/crates.io-index"
[[package]]
name = "winapi-build"
version = "0.1.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
[metadata]
"checksum aho-corasick 0.5.3 (registry+https://github.com/rust-lang/crates.io-index)" = "ca972c2ea5f742bfce5687b9aef75506a764f61d37f8f649047846a9686ddb66"
"checksum cpython 0.1.0 (git+https://github.com/dgrunwald/rust-cpython.git?rev=b35031e2670d7571f03c313cde8fd91105bd5322)" = "<none>"
"checksum kernel32-sys 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)" = "7507624b29483431c0ba2d82aece8ca6cdba9382bff4ddd0f7490560c056098d"
"checksum libc 0.2.34 (registry+https://github.com/rust-lang/crates.io-index)" = "36fbc8a8929c632868295d0178dd8f63fc423fd7537ad0738372bd010b3ac9b0"
"checksum memchr 0.1.11 (registry+https://github.com/rust-lang/crates.io-index)" = "d8b629fb514376c675b98c1421e80b151d3817ac42d7c667717d282761418d20"
"checksum num-traits 0.1.41 (registry+https://github.com/rust-lang/crates.io-index)" = "cacfcab5eb48250ee7d0c7896b51a2c5eec99c1feea5f32025635f5ae4b00070"
"checksum python27-sys 0.1.2 (git+https://github.com/dgrunwald/rust-cpython.git?rev=b35031e2670d7571f03c313cde8fd91105bd5322)" = "<none>"
"checksum regex 0.1.80 (registry+https://github.com/rust-lang/crates.io-index)" = "4fd4ace6a8cf7860714a2c2280d6c1f7e6a413486c13298bbc86fd3da019402f"
"checksum regex-syntax 0.3.9 (registry+https://github.com/rust-lang/crates.io-index)" = "f9ec002c35e86791825ed294b50008eea9ddfc8def4420124fbc6b08db834957"
"checksum thread-id 2.0.0 (registry+https://github.com/rust-lang/crates.io-index)" = "a9539db560102d1cef46b8b78ce737ff0bb64e7e18d35b2a5688f7d097d0ff03"
"checksum thread_local 0.2.7 (registry+https://github.com/rust-lang/crates.io-index)" = "8576dbbfcaef9641452d5cf0df9b0e7eeab7694956dd33bb61515fb8f18cfdd5"
"checksum utf8-ranges 0.1.3 (registry+https://github.com/rust-lang/crates.io-index)" = "a1ca13c08c41c9c3e04224ed9ff80461d97e121589ff27c753a16cb10830ae0f"
"checksum which 1.0.3 (registry+https://github.com/rust-lang/crates.io-index)" = "4be6cfa54dab45266e98b5d7be2f8ce959ddd49abd141a05d52dce4b07f803bb"
"checksum winapi 0.2.8 (registry+https://github.com/rust-lang/crates.io-index)" = "167dc9d6949a9b857f3451275e911c3f44255842c1f7a76f33c55103a909087a"
"checksum winapi-build 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)" = "2d315eee3b34aca4797b2da6b13ed88266e6d612562a0c46390af8299fc699bc"

rust/Cargo.toml

  • This file was added.
[workspace]
members = ["hgcli"]

rust/README.rst

  • This file was added.
===================
Mercurial Rust Code
===================
This directory contains various Rust code for the Mercurial project.
The top-level ``Cargo.toml`` file defines a workspace containing
all primary Mercurial crates.
Building
========
To build the Rust components::
$ cargo build
If you prefer a non-debug / release configuration::
$ cargo build --release
Running
=======
The ``hgcli`` crate produces an ``hg`` binary. You can run this binary
via ``cargo run``::
$ cargo run --manifest-path hgcli/Cargo.toml
Or directly::
$ target/debug/hg
$ target/release/hg

rust/hgcli/Cargo.toml

  • This file was added.
[package]
name = "hgcli"
version = "0.1.0"
authors = ["Gregory Szorc <gregory.szorc@gmail.com>"]
build = "build.rs"
[[bin]]
name = "hg"
path = "src/main.rs"
[dependencies]
libc = "0.2.34"
which = "1.0.3"
[build-dependencies]
which = "1.0.3"
[dependencies.cpython]
version = "0.1"
default-features = false
features = ["python27-sys"]
git = "https://github.com/dgrunwald/rust-cpython.git"
rev = "b35031e2670d7571f03c313cde8fd91105bd5322"

rust/hgcli/build.rs

  • This file was added.
// build.rs -- Configure build environment for `hgcli` Rust package.
//
// Copyright 2017 Gregory Szorc <gregory.szorc@gmail.com>
//
// This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version.
extern crate which;
use std::collections::HashMap;
use std::env;
use std::path::PathBuf;
use std::process::Command;
cramertjUnsubmitted
Done

Nit: if you move this import into have_shared, you'll only need one cfg and it'll be easier to validate that the proper deps are available for the proper platforms.

cramertj: Nit: if you move this import into `have_shared`, you'll only need one `cfg` and it'll be easier…
struct PythonConfig {
python: PathBuf,
config: HashMap<String, String>,
}
fn get_python_config() -> PythonConfig {
let python;
if let Ok(env_python) = env::var("PYTHON_SYS_EXECUTABLE") {
python = PathBuf::from(env_python);
} else {
if let Ok(path_python) = which::which("python2.7") {
python = path_python;
} else {
panic!("could not find python2.7 executable");
}
}
let separator = "SEPARATOR STRING";
kevincoxUnsubmitted
Not Done
assert!(
    Path::new(&python).exists(),
    "Python interpreter {} does not exist; this should never happen",
    python);

I would also recommend {:?} as the quotes highlight the injected variable nicely and it will make some hard-to-notice things more visible due to escaping.

kevincox: ``` assert!( Path::new(&python).exists(), "Python interpreter {} does not exist; this…
let script = "import sysconfig; \
c = sysconfig.get_config_vars(); \
print('SEPARATOR STRING'.join('%s=%s' % i for i in c.items()))";
let mut command = Command::new(&python);
command.arg("-c").arg(script);
ruudaUnsubmitted
Not Done

You can safely indent those, leading whitespace on the continuation line will be stripped from the string literal.

ruuda: You can safely indent those, leading whitespace on the continuation line will be stripped from…
let out = command.output().unwrap();
if !out.status.success() {
panic!(
"python script failed: {}",
String::from_utf8_lossy(&out.stderr)
);
}
let stdout = String::from_utf8_lossy(&out.stdout);
let mut m = HashMap::new();
for entry in stdout.split(separator) {
let mut parts = entry.splitn(2, "=");
let key = parts.next().unwrap();
let value = parts.next().unwrap();
m.insert(String::from(key), String::from(value));
}
PythonConfig {
python: python,
config: m,
}
}
#[cfg(not(target_os = "windows"))]
fn have_shared(config: &PythonConfig) -> bool {
match config.config.get("Py_ENABLE_SHARED") {
Some(value) => value == "1",
None => false,
}
}
#[cfg(target_os = "windows")]
fn have_shared(config: &PythonConfig) -> bool {
// python27.dll should exist next to python2.7.exe.
let mut dll = config.python.clone();
dll.pop();
dll.push("python27.dll");
return dll.exists();
}
fn main() {
let config = get_python_config();
println!("Using Python: {}", config.python.to_string_lossy());
yujaUnsubmitted
Done

Nit: const ?

yuja: Nit: `const` ?
cramertjUnsubmitted
Done

Nit: not sure what version you're targeting, but 'static is automatic for const vars, so you could write [&str; 2]

cramertj: Nit: not sure what version you're targeting, but `'static` is automatic for `const` vars, so…
kevincoxUnsubmitted
Not Done

I would also recommend using a slice if you don't intend the size of the array to be part of the type signature.

const REQUIRED_CONFIG_FLAGS: &[&str] = &["Py_USING_UNICODE", "WITH_THREAD"];
kevincox: I would also recommend using a slice if you don't intend the size of the array to be part of…
let prefix = config.config.get("prefix").unwrap();
println!("Prefix: {}", prefix);
if !have_shared(&config) {
panic!("Detected Python lacks a shared library, which is required");
}
// If building standalone Mercurial, add an extra link path for
// native libraries.
if let Some(lib_path) = env::var_os("HG_STANDALONE_LINK_PATH") {
println!(
"cargo:rustc-link-search=native={}",
lib_path.to_str().unwrap()
);
}
}
kevincoxUnsubmitted
Not Done

Use assert!.

kevincox: Use `assert!`.
kevincoxUnsubmitted
Not Done

Use assert!

kevincox: Use `assert!`
kevincoxUnsubmitted
Not Done
#[cfg(not(target_os = "windows"))]
assert_eq!(
    config.config.get("Py_UNICODE_SIZE"), Some("4"),
     "Detected Python doesn't support UCS-4 code points");
kevincox: ``` #[cfg(not(target_os = "windows"))] assert_eq!( config.config.get("Py_UNICODE_SIZE")…
ruudaUnsubmitted
Not Done

There is no need to match:

let result = config.config.get(*key) == Some("1");

Or using assert as @kevincox recommends:

assert_eq!(config.config.get(*key), Some("1"), "Detected ...");
ruuda: There is no need to match: let result = config.config.get(*key) == Some("1"); Or using…

rust/hgcli/src/main.rs

  • This file was added.
// main.rs -- Main routines for `hg` program
//
// Copyright 2017 Gregory Szorc <gregory.szorc@gmail.com>
//
// This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version.
extern crate libc;
extern crate cpython;
extern crate which;
use cpython::{NoArgs, ObjectProtocol, PyModule, PyResult, Python};
use libc::{c_char, c_int};
use std::env;
use std::path::PathBuf;
use std::ffi::CString;
extern "C" {
pub fn Py_SetPythonHome(arg1: *mut c_char);
pub fn Py_SetProgramName(arg1: *const c_char);
pub fn Py_Initialize();
pub fn Py_Finalize();
pub fn PyEval_InitThreads();
// This actually returns a pointer to a struct.
pub fn PyEval_SaveThread() -> *mut c_char;
pub fn PySys_SetArgv(arg1: c_int, arg2: *const *const c_char) -> ();
pub fn PySys_SetArgvEx(arg1: c_int, arg2: *const *const c_char, arg3: c_int) -> ();
}
#[derive(Debug)]
struct Environment {
quarkUnsubmitted
Done

Maybe use extern crate python27_sys so part of them get defined.

quark: Maybe use `extern crate python27_sys` so part of them get defined.
_exe: PathBuf,
python_exe: PathBuf,
python_home: PathBuf,
mercurial_modules: PathBuf,
}
quarkUnsubmitted
Done

Have you tried rustfmt? It will remove the space before :.

quark: Have you tried `rustfmt`? It will remove the space before `:`.
kevincoxUnsubmitted
Not Done

Use expect for a better error message. .expect("Error getting executable path")

kevincox: Use expect for a better error message. `.expect("Error getting executable path")`
// TODO we probably want to customize the behavior of this function
// based on cargo features/config.
fn get_environment() -> Environment {
let exe_buf = env::current_exe().unwrap();
// Standalone layout is:
// bin/hg
// hgpython/bin/python2.7
cramertjUnsubmitted
Done

Nit: you can just write &str. Also, I'm not familiar with what you're trying to do here, but is the PYTHON_INTERPRETER always determined at compile-time? It seems like something you might want to switch on at runtime. Is that not the case?

cramertj: Nit: you can just write `&str`. Also, I'm not familiar with what you're trying to do here, but…
indygregAuthorUnsubmitted
Done

This is meant to be dynamic. The gist of this code is we're trying to find the location of the Python install given various search strategies. The search strategy is (currently) defined at compile time. And this localdev search strategy defines the path to Python at compile time. Look for my features and documentation for this code later. (I stripped out unused code from this patch to make it smaller.)

indygreg: This is meant to be dynamic. The gist of this code is we're trying to find the location of the…
// hgpython/lib/libpython2.7.so
let mut standalone_root = exe_buf.clone();
standalone_root.pop();
standalone_root.pop();
let mut standalone_python_exe = standalone_root.clone();
standalone_python_exe.push("hgpython");
standalone_python_exe.push("bin");
standalone_python_exe.push("python2.7");
let (is_standalone, python_exe) = match standalone_python_exe.exists() {
true => (true, standalone_python_exe),
// TODO handle failure gracefully.
false => (false, which::which("python2.7").unwrap()),
};
let mut python_home = python_exe.clone();
yujaUnsubmitted
Done

Nit: probably we don't have to clone() here, just move these values.

yuja: Nit: probably we don't have to `clone()` here, just move these values.
python_home.pop();
// On Windows, python2.7.exe exists at the root directory of the Python
// install. Everywhere else, the Python install root is one level up.
if !python_exe.ends_with("python2.7.exe") {
python_home.pop();
}
let mercurial_modules = if is_standalone {
let mut p = standalone_root.clone();
p.push("mercurial");
p
} else {
// rust/target/<build>/hg
let mut p = exe_buf.clone();
p.pop();
p.pop();
p.pop();
p.pop();
p.push("mercurial");
if !p.exists() {
panic!("could not find Mercurial modules");
}
p.pop();
p
};
Environment {
kevincoxUnsubmitted
Not Done

This method allows paths that aren't valid UTF-8 by avoiding ever becoming a str.

CString::new(env.python_home.as_ref().as_bytes())

I would also change the unwrap to .expect("Error setting python home").

kevincox: This method allows paths that aren't valid UTF-8 by avoiding ever becoming a `str`. ```…
_exe: exe_buf.clone(),
python_exe: python_exe.clone(),
python_home: python_home.clone(),
mercurial_modules: mercurial_modules.clone(),
}
}
fn set_python_home(env: &Environment) {
kevincoxUnsubmitted
Not Done

It appears that HG accepts HGUNICODEPEDANTRY= as not enabling unicode pedantry. Maybe the behavior should be the same here. Untested code below should work.

let pedantry = !env::var("HGUNICODEPEDANTRY").unwrap_or("").is_empty();
kevincox: It appears that HG accepts `HGUNICODEPEDANTRY=` as not enabling unicode pedantry. Maybe the…
let raw = CString::new(env.python_home.to_str().unwrap())
yujaUnsubmitted
Done

Perhaps we'll need a utility function for platform-specific cstr
conversion.

On Unix, the story is simple. We can use OsStrExt. On Windows,
maybe we'll have to first convert it to "wide characters" by OsStrExt
and then call WideCharToMultiByte to convert it back to ANSI bytes sequence.

I have no idea if Rust stdlib provides a proper way to convert
OsStr to Windows ANSI bytes.

https://stackoverflow.com/a/38948854
https://msdn.microsoft.com/en-us/library/windows/desktop/dd374130(v=vs.85).aspx

yuja: Perhaps we'll need a utility function for platform-specific cstr conversion. On Unix, the…
indygregAuthorUnsubmitted
Done

I documented a potential path forward in the code. I was thinking we can address this is a follow-up because it is non-trivial to implement. And we may want to do something funky, such as inject the raw bytes into mercurial.argv or something. Since we can call the Windows API directly, we can actually do that now. This opens up a lot of possibilities for encoding...

indygreg: I documented a potential path forward in the code. I was thinking we can address this is a…
.unwrap()
.into_raw();
unsafe {
Py_SetPythonHome(raw);
}
}
fn update_encoding(py: Python, sys_mod: &PyModule) {
// Call sys.setdefaultencoding("undefined") if HGUNICODEPEDANTRY is set.
let pedantry = env::var("HGUNICODEPEDANTRY").is_ok();
quarkUnsubmitted
Done

Maybe use .expect("setdefaultencoding failed").

quark: Maybe use `.expect("setdefaultencoding failed")`.
kevincoxUnsubmitted
Not Done

.expect("Error accessing sys.path")

kevincox: `.expect("Error accessing sys.path")`
// TODO do we need to call reload(sys) here? Should we set Python encoding
// before we start Python interpreter?
if pedantry {
sys_mod
quarkUnsubmitted
Done

Maybe py: Python. That's what rust-cpython uses. It implements Copy.

quark: Maybe `py: Python`. That's what `rust-cpython` uses. It implements `Copy`.
.call(py, "setdefaultencoding", ("undefined",), None)
.expect("sys.setdefaultencoding() failed");
}
}
quarkUnsubmitted
Done

Probably use PyResult<int> since dispatch.run returns the exit code.

quark: Probably use `PyResult<int>` since `dispatch.run` returns the exit code.
indygregAuthorUnsubmitted
Done

`dispatch.run() actually calls sys.exit()`. There is a bug around somewhere in this code around exit code handling. But it only presents in a few tests, which is odd. I documented that in the commit message.

indygreg: ``dispatch.run()`` actually calls ``sys.exit()``. There is a bug around somewhere in this code…
fn update_modules_path(env: &Environment, py: Python, sys_mod: &PyModule) {
let sys_path = sys_mod.get(py, "path").unwrap();
sys_path
.call_method(py, "insert", (0, env.mercurial_modules.to_str()), None)
.expect("failed to update sys.path to location of Mercurial modules");
cramertjUnsubmitted
Done

If it needs to live for the whole time, consider storing it in a static or similar. There's a subtle unsafety here: if this program panics (due to panic, unwrap, or expect, program_name will be dropped (free'd) before the python interpreter is killed (when the process ends, that is-- Finalize won't ever be called in that case). I don't know how much of an issue this will be in practice, but it's something to think about.

cramertj: If it needs to live for the whole time, consider storing it in a `static` or similar. There's a…
}
fn run() -> Result<(), i32> {
let env = get_environment();
durin42Unsubmitted
Done

You might be able to usefully avoid all the unwrap() in here by having an extra layer, eg

fn run() -> Result<(), failure::Error> {
...

let program_name = CString::new(env.python_exe.to_str()?)?.as_ptr();

}

and then

fn main() {

let mut exit_code;
match run() {
  Ok(_) => exit_code = 0,
  Err(e) => { println!("{:?}", e); exit_code = 1; },

}

std::process::exit(exit_code);

}

etc

durin42: You might be able to usefully avoid all the unwrap() in here by having an extra layer, eg fn…
//println!("{:?}", env);
// Tell Python where it is installed.
kevincoxUnsubmitted
Not Done

CString::new(env.python_exe.as_ref().as_bytes())

kevincox: `CString::new(env.python_exe.as_ref().as_bytes())`
yujaUnsubmitted
Not Done

That's more correct on Unix, but wouldn't work on Windows since
native string is UTF-16 variant (Rust) or ANSI (Python 2.7).

yuja: That's more correct on Unix, but wouldn't work on Windows since native string is UTF-16…
kevincoxUnsubmitted
Not Done

Oops, for some reason I thought this was in a unix block. as_bytes() isn't actually available otherise. I think that this is fine then. non-UTF-8 paths shouldn't be a major issue..

kevincox: Oops, for some reason I thought this was in a unix block. `as_bytes()` isn't actually available…
set_python_home(&env);
kevincoxUnsubmitted
Not Done

.expect("Error setting program name")

kevincox: `.expect("Error setting program name")`
// Set program name. The backing memory needs to live for the duration of the
// interpreter.
let program_name = CString::new(env.python_exe.to_str().unwrap())
.unwrap()
.as_ptr();
unsafe {
Py_SetProgramName(program_name);
}
// TODO https://docs.python.org/2/c-api/init.html#c.PySys_SetArgvEx says
// 1. We may wish to not update sys.path as part of setting args
// 2. Initial argument should be empty string since we're not executing a Python script
let args: Vec<CString> = env::args().map(|s| CString::new(s).unwrap()).collect();
let argv: Vec<*const c_char> = args.iter().map(|a| a.as_ptr()).collect();
unsafe {
Py_Initialize();
PySys_SetArgv(args.len() as c_int, argv.as_ptr());
PyEval_InitThreads();
let _thread_state = PyEval_SaveThread();
}
let gil = Python::acquire_gil();
let py = gil.python();
let sys_mod = py.import("sys").unwrap();
update_encoding(py, &sys_mod);
update_modules_path(&env, py, &sys_mod);
// TODO we don't capture exit code from Mercurial.
match run_py(py) {
Err(err) => {
err.print(py);
return Err(255)
}
Ok(()) => Ok(()),
quarkUnsubmitted
Done

255 is probably better since that's an uncaught exception.

quark: `255` is probably better since that's an uncaught exception.
}
}
fn run_py(py: Python) -> PyResult<()> {
let demand_mod = py.import("hgdemandimport")?;
demand_mod.call(py, "enable", NoArgs, None)?;
let dispatch_mod = py.import("mercurial.dispatch")?;
dispatch_mod.call(py, "run", NoArgs, None)?;
Ok(())
}
fn main() {
let exit_code = match run() {
ruudaUnsubmitted
Not Done

There exists Result::map_err for this:

result = run_py(&env, py).map_err(|err| {
    err.print(py);
    255
});
ruuda: There exists `Result::map_err` for this: result = run_py(&env, py).map_err(|err| {…
Err(err) => err,
Ok(()) => 0,
};
// If Python isn't initialized (we could hit an error before
// Py_initialize()), this will no-op. So it is safe to always call.
unsafe {
Py_Finalize();
}
std::process::exit(exit_code);
}
kevincoxUnsubmitted
Not Done

Since this method returns a Result why not handle the error?

let sys_mod = py.import("sys")?;
kevincox: Since this method returns a Result why not handle the error? ``` let sys_mod = py.import…