kevincox added a comment.
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 :)
INLINE COMMENTS
> cramertj wrote in build.rs:88
> Nit: not sure what version you're targeting, but `'static` is automatic for
> `const` vars, so you could write `[&str; 2]`
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"];
> build.rs:33
> + );
> + }
> +
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.
> build.rs:110
> + if !result {
> + panic!("Detected Python requires feature {}", key);
> + }
Use `assert!`.
> build.rs:116
> + if !have_shared(&config) {
> + panic!("Detected Python lacks a shared library, which is required");
> + }
Use `assert!`
> build.rs:127
> + panic!("Detected Python doesn't support UCS-4 code points");
> + }
> +}
#[cfg(not(target_os = "windows"))]
assert_eq!(
config.config.get("Py_UNICODE_SIZE"), Some("4"),
"Detected Python doesn't support UCS-4 code points");
> main.rs:37
> +fn get_environment() -> Environment {
> + let exe = env::current_exe().unwrap();
> +
Use expect for a better error message. `.expect("Error getting executable
path")`
> main.rs:91
> + .unwrap()
> + .into_raw();
> + unsafe {
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")`.
> main.rs:99
> + // Call sys.setdefaultencoding("undefined") if HGUNICODEPEDANTRY is set.
> + let pedantry = env::var("HGUNICODEPEDANTRY").is_ok();
> +
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();
> main.rs:111
> +fn update_modules_path(env: &Environment, py: Python, sys_mod: &PyModule) {
> + let sys_path = sys_mod.get(py, "path").unwrap();
> + sys_path
`.expect("Error accessing sys.path")`
> main.rs:133
> + // not desirable.
> + let program_name = CString::new(env.python_exe.to_str().unwrap())
> + .unwrap()
`CString::new(env.python_exe.as_ref().as_bytes())`
> main.rs:134
> + let program_name = CString::new(env.python_exe.to_str().unwrap())
> + .unwrap()
> + .as_ptr();
`.expect("Error setting program name")`
> main.rs:200
> +fn run_py(env: &Environment, py: Python) -> PyResult<()> {
> + let sys_mod = py.import("sys").unwrap();
> +
Since this method returns a Result why not handle the error?
let sys_mod = py.import("sys")?;
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D1581
To: indygreg, #hg-reviewers, yuja, durin42
Cc: kevincox, cramertj, yuja, quark, durin42, dlax, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
