-
Notifications
You must be signed in to change notification settings - Fork 346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reftests: handle cygwin paths #5286
Conversation
9eebb1e
to
e46cc0c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the effect on the tests! Some possible tweaks to exactly what we do with the paths
OpamSystem.read_command_output ([ "cygpath" ] @ args) | ||
with | ||
| x::_ when not (String.equal x "") && f x -> | ||
Some OpamSystem.(back_to_forward x, forward_to_back x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels slightly over-kill. cygpath -u
will always return forward-slashes and cygpath -w
will always return back-slashes.
(* Some outputs contain both Windows-like path C:\ and cygwin-like path /cygdrive/c *) | ||
let cygwinpath = | ||
match get_cygwinpath dir with | ||
| Some (cwp_fwd, cwp_bck) -> [ str cwp_fwd ; str cwp_bck ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should further normalise Cygwin-paths containing backslashes (that feels like an error somewhere)
in | ||
let winpath = | ||
match get_winpath dir with | ||
| Some (wp_fwd, wp_bck) -> [ str wp_fwd ; str wp_bck ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like us to get to the stage where we only see properly formatted Windows paths, but supporting all \
and all /
but not mixed seems a little strange. How about this instead: we know that get_winpath
will return only backslashes, so we can split the string on \
and then construct seq [ component1 (set "\\/") component2 (set "\\/") .. ]
?
Co-authored-by: David Allsopp <david.allsopp@metastack.com>
supersede by #5723 |
Some windows paths were not prefixed with cygwin path, and then not catch by path regexp.
Also, for some tests we get
PATH
from environment that is in cygwin style whileSys.getcwd
gives the windows style one. Add inBASEDIR
regexp both style.