This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8937

rust: enable on macOS
AbandonedPublic

Authored by danchr on Aug 19 2020, 11:52 AM.

Details

Reviewers
Alphare
marmoute
Group Reviewers
hg-reviewers
Summary

AFAICT this works quite well, apart from one issue related to the
dirstate mishandling removed files. In particular,
tests/test-casefolding.t will have a lot of errors like this:

File ".../mercurial/dirstate.py", line 1888, in filefoldmap
  return self._rustmap.filefoldmapasdict()

TypeError: unhashable type: 'list'

Rust has fairly good support for obtaining the canonical path to a
file, so I just used that. With this change, the test still fails, but
now it just fails due to a subtle difference between the two
implementations:

  • With the Python implementation, doing mv thefile TheFile does not affect hg status.
  • With the Rust implementation, the results are the same as on a case-sensitive file system; one file is missing and the other is unknown.

Please note that this assumes a case preserving, but case insensitive
file system -- excluding FAT, I suspect.

Most of the Rust tests pass as well, apart from one PathAuditor test.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

danchr created this revision.Aug 19 2020, 11:52 AM
Alphare requested changes to this revision.Aug 21 2020, 4:24 AM
Alphare added a subscriber: Alphare.

What do you mean by "mishandling removed files"? The fold map is there to give a mapping between the normalized (internal) representation of file paths and the actual ones on disk. As much as I hate the dirstate code, this is a useful part of the software that is triggered depending on your filesystem and paths used.

I don't have a machine running MacOS to run the tests on, all I know is that if the repo root is on a FS that is case-insensitive, the Rust code will not be run. Also, the PathAuditor failing is not a good sign.

I am currently rewriting some of the dirstate code, hopefully my experiment can make it upstream and improve the code, but I don't think "official" support for MacOS can come before we get a CI running all the tests and at least someone dogfooding it a little.

This revision now requires changes to proceed.Aug 21 2020, 4:24 AM
danchr added a comment.EditedAug 21 2020, 11:49 AM
In D8937#133741, @Alphare wrote:

What do you mean by "mishandling removed files"? The fold map is there to give a mapping between the normalized (internal) representation of file paths and the actual ones on disk. As much as I hate the dirstate code, this is a useful part of the software that is triggered depending on your filesystem and paths used.

Yeah, after posting this patch, I looked at testruns with and without this patch. I had one extra failure: test-casefolding.t. So it's obviously not correct 😕

I don't have a machine running MacOS to run the tests on, all I know is that if the repo root is on a FS that is case-insensitive, the Rust code will not be run. Also, the PathAuditor failing is not a good sign.
I am currently rewriting some of the dirstate code, hopefully my experiment can make it upstream and improve the code, but I don't think "official" support for MacOS can come before we get a CI running all the tests and at least someone dogfooding it a little.

I've been trying out, and as far as I can tell, the main issue is with the PathAuditor. Other than that, everything appears to work. I'd like to investigate further, but I'm not sure how to debug the Rust code. The error I get is this:

--- .../tests/test-casefolding.t
+++ .../tests/test-casefolding.t.err
@@ -34,13 +34,226 @@
 
   $ hg mv a A
   $ hg mv A a
+  ** unknown exception encountered, please report by visiting
+  ** https://mercurial-scm.org/wiki/BugTracker
+  ** Python 3.8.5 (default, Jul 21 2020, 18:31:18) [Clang 11.0.3 (clang-1103.0.32.62)]
+  ** Mercurial Distributed SCM (version 5.4.1+168-120ca13cb67c+20200821)
+  ** Extensions loaded: 
+  Traceback (most recent call last):
[snip]
+    File ".../mercurial/dirstate.py", line 602, in normalize
+      return self._normalize(path, isknown, ignoremissing)
+    File ".../mercurial/dirstate.py", line 569, in _normalize
+      folded = self._map.filefoldmap.get(normed, None)
+    File ".../mercurial/util.py", line 1744, in __get__
+      result = self.func(obj)
+    File ".../mercurial/dirstate.py", line 1888, in filefoldmap
+      return self._rustmap.filefoldmapasdict()
+  TypeError: unhashable type: 'list'
+  [1]

Could you perhaps offer some pointers on how I might investigate what's going wrong?

danchr edited the summary of this revision. (Show Details)Aug 24 2020, 3:52 PM
danchr updated this revision to Diff 22434.
Alphare added a comment.Aug 25 2020, 3:55 AM
In D8937#133743, @danchr wrote:

I'd like to investigate further, but I'm not sure how to debug the Rust code. The error I get is this:

--- .../tests/test-casefolding.t
+++ .../tests/test-casefolding.t.err
@@ -34,13 +34,226 @@
   $ hg mv a A
   $ hg mv A a
+  ** unknown exception encountered, please report by visiting
+  ** https://mercurial-scm.org/wiki/BugTracker
+  ** Python 3.8.5 (default, Jul 21 2020, 18:31:18) [Clang 11.0.3 (clang-1103.0.32.62)]
+  ** Mercurial Distributed SCM (version 5.4.1+168-120ca13cb67c+20200821)
+  ** Extensions loaded: 
+  Traceback (most recent call last):
[snip]
+    File ".../mercurial/dirstate.py", line 602, in normalize
+      return self._normalize(path, isknown, ignoremissing)
+    File ".../mercurial/dirstate.py", line 569, in _normalize
+      folded = self._map.filefoldmap.get(normed, None)
+    File ".../mercurial/util.py", line 1744, in __get__
+      result = self.func(obj)
+    File ".../mercurial/dirstate.py", line 1888, in filefoldmap
+      return self._rustmap.filefoldmapasdict()
+  TypeError: unhashable type: 'list'
+  [1]

Could you perhaps offer some pointers on how I might investigate what's going wrong?

Some of the code infrastructure needed to support case-insensitive filesystems such as the filefoldmap is in place, but only because it mean little to no effort on my part or because some Python code expected the attribute to be there. Having never run the tests on a case-insensitive FS (or really ever been logged in to a machine with one in the last 10 years), I cannot say whether the current code works, it's more of a best effort. It's not that much code: I did not write the normalization routines for example.

The error you're having looks like should be fixed by the change you've proposed. I have no simple way of testing that, though. There is a util in rust/hg-cpython/src/utils.py called print_python_trace that could help you figure out where the Rust code is being called from that I've used from time to time. Other than that I've mostly looked at the Python/C code to replicate/adapt what was being done and looked at the test output for failures.

To recap: there is quite a bit of code that is missing in order to make the code work on case-insensitive filesystems. You should grep for "TODO" in Rust code, I tried to add one any time I skipped some logic.

marmoute requested changes to this revision.Aug 26 2020, 12:58 PM
marmoute added a subscriber: marmoute.

Looks like we should take the change to dirstate_map.rs (in its own changesets) and delay the change to rust/hg-core/src/lib.rs, right? @danchr can you split this?

@Alphare friendly reminder that your employer provide you access to a Mac OS machine.

This revision now requires changes to proceed.Aug 26 2020, 12:58 PM
danchr edited the summary of this revision. (Show Details)Aug 26 2020, 1:31 PM
danchr updated this revision to Diff 22464.
pulkit added a subscriber: pulkit.Sep 8 2020, 3:21 AM

@Alphare seems like this one needs a re-look.

Alphare added a comment.Sep 10 2020, 4:33 AM

Thanks for taking the time to do this.

I however still have reservations as to adding MacOS as a supported platform since it currently is tested in no CI that runs even semi-frequently on Rust code.
Also, what do you mean by "most tests pass"? What is needed to make the rest pass? And finally, which is the correct behavior from the Python version or the Rust one (according to your new commit message)?

I will take time to take a look at the code in detail after this becomes clearer for me, just so I can understand what compromise, if any, you're making right now.

rust/hg-cpython/src/dirstate/dirstate_map.rs
353

As stated by @marmoute, this should be its own patch, because the code is just broken without it and it can be accepted without the weight of the rest.

marmoute added a comment.Sep 17 2020, 3:53 AM
In D8937#134836, @Alphare wrote:

Thanks for taking the time to do this.
I however still have reservations as to adding MacOS as a supported platform since it currently is tested in no CI that runs even semi-frequently on Rust code.
Also, what do you mean by "most tests pass"? What is needed to make the rest pass? And finally, which is the correct behavior from the Python version or the Rust one (according to your new commit message)?
I will take time to take a look at the code in detail after this becomes clearer for me, just so I can understand what compromise, if any, you're making right now.

@Alphare do you think you could produce a list of necessary step to be taken before we can run this

marmoute added a comment.Sep 23 2020, 2:41 AM

@Alphare gentle ping to provide a todo list here.

Alphare added a comment.Sep 30 2020, 4:25 AM
In D8937#135966, @marmoute wrote:

@Alphare gentle ping to provide a todo list here.

I'm not sure what you mean, I've asked questions precisely because I don't have a clear idea of what's going on.

pulkit added a comment.Dec 4 2020, 2:09 PM

I will very much like to see this move forward.

@Alphare IIUC, your concern is about not having a CI where rust+MacOS is tested regularly and remaining of test failures, right?

Alphare added a comment.Dec 7 2020, 4:24 AM
In D8937#143136, @pulkit wrote:

I will very much like to see this move forward.
@Alphare IIUC, your concern is about not having a CI where rust+MacOS is tested regularly and remaining of test failures, right?

Correct. I'm also not sure about using std::fs::canonicalize, since it does a filesystem check every time and has issues (https://www.reddit.com/r/rust/comments/jqx8hf/normpath_more_reliable_path_manipulation/)

danchr abandoned this revision.Dec 20 2020, 1:37 PM
danchr added a comment.Dec 21 2020, 2:10 PM

I actually hadn't noticed that you kept discussing this prior to abandoning it. I'm busy with other things, though, so I won't be able to follow up on this.

danchr added a comment.Dec 30 2020, 7:02 AM
In D8937#134080, @marmoute wrote:

Looks like we should take the change to dirstate_map.rs (in its own changesets) and delay the change to rust/hg-core/src/lib.rs, right? @danchr can you split this?

Done, it's .D9671 — and I managed to fix the test failure while at it.

Revision Contents
Changeset List

Diff 22464

mercurial/dirstate.py

allowed_matchers = ( allowed_matchers = (
matchmod.alwaysmatcher, matchmod.alwaysmatcher,
matchmod.exactmatcher, matchmod.exactmatcher,
matchmod.includematcher, matchmod.includematcher,
) )
if rustmod is None: if rustmod is None:
use_rust = False use_rust = False
elif self._checkcase:
# Case-insensitive filesystems are not handled yet
use_rust = False
elif subrepos: elif subrepos:
use_rust = False use_rust = False
elif sparse.enabled: elif sparse.enabled:
use_rust = False use_rust = False
elif not isinstance(match, allowed_matchers): elif not isinstance(match, allowed_matchers):
# Some matchers have yet to be implemented # Some matchers have yet to be implemented
use_rust = False use_rust = False

rust/hg-core/src/dirstate/dirstate_map.rs

// dirstate_map.rs // dirstate_map.rs
// //
// Copyright 2019 Raphaël Gomès <rgomes@octobus.net> // Copyright 2019 Raphaël Gomès <rgomes@octobus.net>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
use crate::{ use crate::{
dirstate::{parsers::PARENT_SIZE, EntryState, SIZE_FROM_OTHER_PARENT}, dirstate::{parsers::PARENT_SIZE, EntryState, SIZE_FROM_OTHER_PARENT},
pack_dirstate, parse_dirstate, pack_dirstate, parse_dirstate,
utils::{ utils::hg_path::{HgPath, HgPathBuf},
files::normalize_case,
hg_path::{HgPath, HgPathBuf},
},
CopyMap, DirsMultiset, DirstateEntry, DirstateError, DirstateMapError, CopyMap, DirsMultiset, DirstateEntry, DirstateError, DirstateMapError,
DirstateParents, DirstateParseError, FastHashMap, StateMap, DirstateParents, DirstateParseError, FastHashMap, StateMap,
}; };
use core::borrow::Borrow; use core::borrow::Borrow;
use std::collections::HashSet; use std::collections::HashSet;
use std::convert::TryInto; use std::convert::TryInto;
use std::iter::FromIterator; use std::iter::FromIterator;
use std::ops::Deref; use std::ops::Deref;
} }
if old_state == EntryState::Unknown { if old_state == EntryState::Unknown {
if let Some(ref mut all_dirs) = self.all_dirs { if let Some(ref mut all_dirs) = self.all_dirs {
all_dirs.add_path(filename)?; all_dirs.add_path(filename)?;
} }
} }
if let Some(ref mut file_fold_map) = self.file_fold_map { if let Some(ref mut file_fold_map) = self.file_fold_map {
file_fold_map.remove(&normalize_case(filename)); match filename.canonical_name() {
Ok(canonical_name) => {
file_fold_map.remove(&canonical_name);
}
Err(_) => {}
}
} }
self.state_map.insert( self.state_map.insert(
filename.to_owned(), filename.to_owned(),
DirstateEntry { DirstateEntry {
state: EntryState::Removed, state: EntryState::Removed,
mode: 0, mode: 0,
size, size,
mtime: 0, mtime: 0,
dirs.delete_path(filename)?; dirs.delete_path(filename)?;
} }
} }
if let Some(ref mut all_dirs) = self.all_dirs { if let Some(ref mut all_dirs) = self.all_dirs {
all_dirs.delete_path(filename)?; all_dirs.delete_path(filename)?;
} }
} }
if let Some(ref mut file_fold_map) = self.file_fold_map { if let Some(ref mut file_fold_map) = self.file_fold_map {
file_fold_map.remove(&normalize_case(filename)); match filename.canonical_name() {
Ok(canonical_name) => {
file_fold_map.remove(&canonical_name);
}
Err(_) => {}
}
} }
self.get_non_normal_other_parent_entries() self.get_non_normal_other_parent_entries()
.0 .0
.remove(filename); .remove(filename);
Ok(exists) Ok(exists)
} }
pub fn build_file_fold_map(&mut self) -> &FileFoldMap { pub fn build_file_fold_map(&mut self) -> &FileFoldMap {
if let Some(ref file_fold_map) = self.file_fold_map { if let Some(ref file_fold_map) = self.file_fold_map {
return file_fold_map; return file_fold_map;
} }
let mut new_file_fold_map = FileFoldMap::default(); let mut new_file_fold_map = FileFoldMap::default();
for (filename, DirstateEntry { state, .. }) in self.state_map.borrow() for (filename, DirstateEntry { state, .. }) in self.state_map.borrow()
{ {
if *state == EntryState::Removed { if *state == EntryState::Removed {
match filename.canonical_name() {
Ok(canonical_filename) => {
new_file_fold_map new_file_fold_map
.insert(normalize_case(filename), filename.to_owned()); .insert(filename.to_owned(), canonical_filename);
}
Err(_) => {
}
}
} }
} }
self.file_fold_map = Some(new_file_fold_map); self.file_fold_map = Some(new_file_fold_map);
self.file_fold_map.as_ref().unwrap() self.file_fold_map.as_ref().unwrap()
} }
} }
#[cfg(test)] #[cfg(test)]

rust/hg-core/src/dirstate/status.rs

// TODO don't collect. Find a way of replicating the behavior of // TODO don't collect. Find a way of replicating the behavior of
// `itertools::process_results`, but for `rayon::ParallelIterator` // `itertools::process_results`, but for `rayon::ParallelIterator`
let new_results: IoResult<Vec<_>> = to_visit let new_results: IoResult<Vec<_>> = to_visit
.into_par_iter() .into_par_iter()
.filter_map(|(filename, entry)| -> Option<IoResult<_>> { .filter_map(|(filename, entry)| -> Option<IoResult<_>> {
// Report ignored items in the dmap as long as they are not // Report ignored items in the dmap as long as they are not
// under a symlink directory. // under a symlink directory.
if path_auditor.check(filename) { // Only consider items found under the proper name
// TODO normalize for case-insensitive filesystems if filename.check_name() && path_auditor.check(filename) {
let buf = match hg_path_to_path_buf(filename) { let buf = match hg_path_to_path_buf(filename) {
Ok(x) => x, Ok(x) => x,
Err(e) => return Some(Err(e.into())), Err(e) => return Some(Err(e.into())),
}; };
Some(Ok(( Some(Ok((
Cow::Borrowed(filename), Cow::Borrowed(filename),
match root_dir.as_ref().join(&buf).symlink_metadata() { match root_dir.as_ref().join(&buf).symlink_metadata() {
// File was just ignored, no links, and exists // File was just ignored, no links, and exists

rust/hg-core/src/lib.rs

}; };
mod filepatterns; mod filepatterns;
pub mod matchers; pub mod matchers;
pub mod revlog; pub mod revlog;
pub use revlog::*; pub use revlog::*;
pub mod operations; pub mod operations;
pub mod utils; pub mod utils;
// Remove this to see (potential) non-artificial compile failures. MacOS // Remove this to see (potential) non-artificial compile failures.
// *should* compile, but fail to compile tests for example as of 2020-03-06 #[cfg(not(any(target_os = "linux", target_os = "macos")))]
#[cfg(not(target_os = "linux"))]
compile_error!( compile_error!(
"`hg-core` has only been tested on Linux and will most \ "`hg-core` has only been tested on Linux and macOS and will most \
likely not behave correctly on other platforms." likely not behave correctly on other platforms."
); );
use crate::utils::hg_path::{HgPathBuf, HgPathError}; use crate::utils::hg_path::{HgPathBuf, HgPathError};
pub use filepatterns::{ pub use filepatterns::{
parse_pattern_syntax, read_pattern_file, IgnorePattern, parse_pattern_syntax, read_pattern_file, IgnorePattern,
PatternFileWarning, PatternSyntax, PatternFileWarning, PatternSyntax,
}; };

rust/hg-core/src/utils/hg_path.rs

// hg_path.rs // hg_path.rs
// //
// Copyright 2019 Raphaël Gomès <rgomes@octobus.net> // Copyright 2019 Raphaël Gomès <rgomes@octobus.net>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
use std::borrow::Borrow; use std::borrow::Borrow;
use std::fs::canonicalize;
use std::ffi::{OsStr, OsString}; use std::ffi::{OsStr, OsString};
use std::fmt; use std::fmt;
use std::ops::Deref; use std::ops::Deref;
use std::path::{Path, PathBuf}; use std::path::{Path, PathBuf};
#[derive(Debug, Eq, PartialEq)] #[derive(Debug, Eq, PartialEq)]
pub enum HgPathError { pub enum HgPathError {
/// Bytes from the invalid `HgPath` /// Bytes from the invalid `HgPath`
} }
pub fn trim_trailing_slash(&self) -> &Self { pub fn trim_trailing_slash(&self) -> &Self {
Self::new(if self.inner.last() == Some(&b'/') { Self::new(if self.inner.last() == Some(&b'/') {
&self.inner[..self.inner.len() - 1] &self.inner[..self.inner.len() - 1]
} else { } else {
&self.inner[..] &self.inner[..]
}) })
} }
pub fn canonical_name(&self) -> Result<HgPathBuf, HgPathError> {
match canonicalize(&hg_path_to_path_buf(&self)?) {
Ok(canonical_path) => {
let canonical_name = os_string_to_hg_path_buf(
&canonical_path.file_name().unwrap()
)?;
Ok(self.parent().join(&canonical_name))
}
Err(_) => Err(HgPathError::NotFsCompliant(self.to_hg_path_buf())),
}
}
pub fn check_name(&self) -> bool {
let path;
match hg_path_to_path_buf(&self) {
Ok(p) => path = p,
Err(_) => return true,
}
match canonicalize(&path) {
Ok(canonical_path) => {
canonical_path.file_name() == path.file_name()
}
Err(_) => true
}
}
/// Returns a tuple of slices `(base, filename)` resulting from the split /// Returns a tuple of slices `(base, filename)` resulting from the split
/// at the rightmost `/`, if any. /// at the rightmost `/`, if any.
/// ///
/// # Examples: /// # Examples:
/// ///
/// ``` /// ```
/// use hg::utils::hg_path::HgPath; /// use hg::utils::hg_path::HgPath;
/// ///

rust/hg-cpython/src/dirstate/dirstate_map.rs

def filefoldmapasdict(&self) -> PyResult<PyDict> { def filefoldmapasdict(&self) -> PyResult<PyDict> {
let dict = PyDict::new(py); let dict = PyDict::new(py);
for (key, value) in for (key, value) in
self.inner(py).borrow_mut().build_file_fold_map().iter() self.inner(py).borrow_mut().build_file_fold_map().iter()
{ {
dict.set_item( dict.set_item(
py, py,
key.as_bytes().to_vec(), PyBytes::new(py, key.as_bytes()).into_object(),
AlphareUnsubmitted
Not Done

As stated by @marmoute, this should be its own patch, because the code is just broken without it and it can be accepted without the weight of the rest.

Alphare: As stated by @marmoute, this should be its own patch, because the code is just broken without…
value.as_bytes().to_vec(), PyBytes::new(py, value.as_bytes()).into_object(),
)?; )?;
} }
Ok(dict) Ok(dict)
} }
def __len__(&self) -> PyResult<usize> { def __len__(&self) -> PyResult<usize> {
Ok(self.inner(py).borrow().len()) Ok(self.inner(py).borrow().len())
} }