-
Notifications
You must be signed in to change notification settings - Fork 371
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
init: install cygwin internally #5545
Conversation
0d12eab
to
cf861fc
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.
This will need lots of foolproof testing, but the patch is good! I only propose a few message rewords.
let required_packages_for_cygwin = | ||
[ | ||
"diffutils"; | ||
"git"; (* XXX hg & mercurial ? *) |
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.
Those are so little used nowadays that we shouldn't bother with them by default
cf861fc
to
83338d3
Compare
src/client/opamCommands.ml
Outdated
let cygwin_internal = | ||
mk_vflag ~cli `none [ | ||
cli_from cli2_2, `internal, ["cygwin-internal-install"], | ||
"Let opam setup and manage an internal Cygwin install"; | ||
cli_from cli2_2, `default_location, ["cygwin-local-install"], | ||
"Use preexistent Cygwin install"; | ||
cli_from cli2_2, `no, ["no-cygwin-setup"], | ||
"Don't setup Cygwin"; | ||
] | ||
in | ||
let cygwin_location = | ||
mk_opt ~cli (cli_from cli2_2) ["cygwin-location"] "DIR" | ||
"Specify Cygwin root location" | ||
Arg.(some dirname) None | ||
in |
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.
Other proposal: --cygwin [no|internal|default|<location>]
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.
Hm I like the proposal (but it's ok as is)
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.
Additional question: do we make sure that the cygwin option don't show up on other OSes ?
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.
now, we do
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.
aside from the minor indentation change, lgtm (on a code standpoint only, i haven't tested on an actual windows machine)
29e4c17
to
66ba38f
Compare
* --cygwin-internal: setup internal cygwin install * --cygwin-install: use local install, default is C:\cygwin64 * --cygwin-location: specify cygwin root path * --no-cygwin-setup: don't setup cygwin in opam
c7e38eb
to
2858c2e
Compare
At init, add an item in the cygwin menu to propose to have a cygwin install internal to opam. It is handled by opam itself (prefix stored in
<opamroot>/.cygwin/root
), when depext are requested, there is no confirmation requested.It also adds some flags to
init
, to have a cywgin configuration non interactively:--no-cygwin-setup
|--cygwin-internal-install
|--cygwin-local-install
and--cygwin-location <path>
.These are experimental as they can change name from the finale release.
TODO