Skip to content
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

Display paths using backslashes on Windows #658

Closed
wants to merge 1 commit into from

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Jul 5, 2016

Packaging up myriad OCaml versions ready for native Windows OPAM has finally made me fix a nagging irritation of the last decade or so...!

The premise is simple: while they are supported, professional Windows applications do not use forward-slashes in paths. Period.

This patch:

  • Changes the generation of utils/config.ml to translate forward-slashes (which are legitimately needed in the Unix-based build system) to back-slashes, meaning that ocamlc -config (and ocamlc -v) display the default standard library location "correctly".
  • Does the same when generating byterun/ld.conf
  • While there, also sw9tches to Windows line-endings for ld.conf

A few minor observations:

  • The fact that Windows supports forward-slashes in paths is a fact known only by quite advanced Windows users and Unix users who are forced to target their software to Windows!
  • Having the standard library location use back-slashes is a good torture test for poorly written package configure scripts (see, for example, the excellent handling in findlib's configure script for how this should be done)
  • While there can't be (m)any programmers relying on Notepad.exe, it doesn't process files with Unix line-endings, which is a mild irritation for editing ld.conf.
  • May I claim a prize for the largest number of backslashes needed in a row in a Makefile?

@dbuenzli
Copy link
Contributor

dbuenzli commented Jul 5, 2016

Do you know if this affects ocamlbuild in any way ? FWIW I recently had to remove uses of Filename.dir_sep to improve Windows compatibility... (dbuenzli/topkg@4c8480e)

@@ -373,7 +373,7 @@ otherlibs/dynlink/dynlink.cmxa: otherlibs/dynlink/natdynlink.ml

utils/config.ml: utils/config.mlp config/Makefile
@rm -f utils/config.ml
sed -e "s|%%LIBDIR%%|$(LIBDIR)|" \
sed -e "s|%%LIBDIR%%|$(shell echo $(LIBDIR)|sed -e 's/\//\\\\\\\\\\\\\\\\/g')|" \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this fine escape sequence could be preceded by a comment reminding the reader what it does.
Also, it might be clearer if another character (e.g. colon) were used as the sed command delimiter, as then you will remove one backslash. The same applies below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@dra27
Copy link
Member Author

dra27 commented Jul 5, 2016

In principle, it shouldn't make any difference - ocamlbuild after 3.11.1 was already using Filename.concat for the limited place where it's used (ocamlbuild -where).
All of this is being back-ported - I have an opam-admin.top script which converts all the compilers to Windows, so at least this will be testable via CI soon.
Usually, the issue is that the backslashes need escaping (and often more than once, because of shell invocations) - so internally converting to forward-slashes is often most sensible, even if it's not how it should have been designed.
However, it's always been valid to have OCAMLLIB with backslashes, so at least the outermost stage of building a package should take that into account.

In order to keep life sane, config/Makefile sensibly uses
forward-slashes for the Windows builds (e.g. PREFIX=C:/OCaml).

While Windows supports paths with either forward or back-slashes (even
permitting them to be mixed in the same path), forward-slashes look
confusing to non-expert Windows users and mixing the two looks extremely
amateur.

Patch ensures that when LIBDIR is displayed (in ocamlc -v, ocamlc
-config, etc.) that forward-slashes are converted to back-slashes.

It also corrects the generation of ld.conf to use them.
@xavierleroy
Copy link
Contributor

I am seriously concerned that you're making the life of OCaml Windows users (even) harder with this change. Backslashes are a quotation nightmare, and most of OCaml Windows software is built using Cygwin.

@dra27
Copy link
Member Author

dra27 commented Jul 5, 2016

If so, should it not be the other way and ensuring that OCAMLLIB is translated to forward slashes when read from the environment? Yes, it's potentially painful, but:

  • Adapting configure scripts is easy; tools like OASIS already work correctly with it
  • The next thing to be (continuing) to address is paths with spaces in them, which are also badly handled (though not any more in OCaml, I know...)

For management, OPAM will reduce the exposure of problems to the (Windows) maintainer for any given package, because the patches will already be there (and hopefully heading upstream)

@@ -373,7 +373,8 @@ otherlibs/dynlink/dynlink.cmxa: otherlibs/dynlink/natdynlink.ml

utils/config.ml: utils/config.mlp config/Makefile
@rm -f utils/config.ml
sed -e "s|%%LIBDIR%%|$(LIBDIR)|" \
# Convert forward-slashes in LIBDIR to backslashes
sed -e "s|%%LIBDIR%%|$(shell echo $(LIBDIR)|sed -e 's:/:\\\\\\\\\\\\\\\\:g')|" \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could simplify this to:

sed -e "s|%%LIBDIR%%|$(subst /,\\\\,$(LIBDIR))|" \

and avoid problems with spaces at the beginning and end of $(LIBDIR).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh - seeing the wood for the trees! Can't believe I didn't think to use $(subst ..) instead! I'll push a new version later.

@damiendoligez
Copy link
Member

About the concerns raised by @xavierleroy, we should ask @alainfrisch.

@dra27
Copy link
Member Author

dra27 commented Jul 7, 2016

I've also, as of yesterday, got OPAM 2.0 capable of installing all 80 Windows versions of OCaml since 3.07 (9GB!) which are all patched with this GPR, so the testing against packages out there can hopefully be considerable...

@alainfrisch
Copy link
Contributor

About the concerns raised by @xavierleroy, we should ask @alainfrisch

I don't really have an opinion. Looks like a low-risk and low-benefit proposal. Low-risk, because a package broken by backslashes would probably be broken already, but it's hard to be sure.

I'm tempted to follow a non-technical argument: @dra27 does a marvelous job at improving support for Windows and seems confident in this one, so I would trust his judgment.

@fdopen
Copy link
Contributor

fdopen commented Jul 7, 2016

The number of windows users is far too low to enforce coding standards for configure scripts, test scripts. etc.

You often see something like this in configure scripts and similar locations:

-I`ocamlc  -where`

As soon as OCAMLLIB contains backslashes, all such constructs become problematic (depending on the shell you are using).
And no, they usually won't merge your patch upstream, because most couldn't care less about windows support. If you want intentionally break such workarounds, you have to maintain your patches yourself.

@dra27
Copy link
Member Author

dra27 commented Jul 7, 2016

That example is already in trouble, regardless of forward or backslashes - it's not handling the \r which will also be in the output of ocamlc -where. -I`ocamlc -where | tr -d "\r"` works with either forward or backslashes.

The problem only occurs when the result is written, say, by a configure script to a Makefile (or Makefile.config) - but again, you already needed to do something to get rid of the the \r, so the processing required either to escape the backslashes properly, or translate them to forward slashes is not exactly much extra, given that work had to be done already.

While I know you've ported many more packages than I have, I've never had trouble upstreaming fixes - although the patches often take months/years actually to be merged, sometimes! It's also often generic build systems which need fixing - e.g. OCamlMakefile, ocaml-autoconf, ocamlbuild (all of which I have had Windows-related patches accepted for), so at least the fixes in one often benefit another. A few other thoughts for why we might hope that the past doesn't indicate the future:

  • OPAM makes maintaining these patches trivial compared with the stress it's been up to now (for example, I almost don't care if Display paths using backslashes on Windows #658 is never merged - it's being automatically applied to any installation of any version of OCaml I make; the worst is that I may need a fresh version of the patch in future - and CI can tell me the second someone makes a commit to OCaml trunk causing that to happen)
  • A combination of OPAM on the horizon and better/available Windows CI I think is shifting opinion of package maintainers where Windows support is concerned. I think the principal reason for not caring about Windows patches in the past was because the package author/principal maintainer could not test and update them themselves.
  • As a minor semantic niggle, this isn't about enforcing coding standards, as much as a bug fix - a path with backslashes (as one with spaces) is a valid, and common occurrence: a build system which doesn't support them is broken!
  • If we happily accept that OPAM is doing all this, it's quite possible that OPAM can perform a behind-the-scenes translation if that really fixes vast numbers of packages. My Windows OPAM is already doing a lot of environment setup when it invokes a Cygwin process (e.g. a configure script, which it starts in a kernel-like manner, not via bash -c): translating the odd extra environment variable would be trivial, though I'm hoping that such hacks can be avoided. But that's not a native-Windows-build-of-OCaml's problem, that should be OPAM's/any other build system/package management system.
  • MSVC-world (I mean proper MSVC world - using nmake and other Microsoft equivalent tools) has the opposite problem. For example, the Tcl/Tk nmake Makefile's have to ensure that forward-slashes in valid paths are always translated to backslashes to prevent the risk of their being interpreted as command line switches. Worth remembering that Windows OCaml is more than mingw...

@xavierleroy
Copy link
Contributor

Six months later, do we better understand the problem we're trying to solve here and how much breakage the solution can cause? Because I still don't.

@dra27
Copy link
Member Author

dra27 commented Dec 4, 2016

It's still ticking over - the Windows ports in https://github.com/dra27/opam-repository/tree/next-windows all use this patch... it should therefore be getting much more thrashing over the next few months. I'm expecting that it will be for after 4.05 (assuming it makes it!)

@dra27 dra27 added this to the 4.06-or-later milestone Dec 29, 2016
@dra27 dra27 modified the milestones: 4.07-or-later, 4.06.0 Aug 12, 2017
@damiendoligez damiendoligez removed this from the consider-for-4.07 milestone Jun 1, 2018
@XVilka
Copy link
Contributor

XVilka commented Sep 20, 2019

@dra27 should be this PR closed then? Since there seem no decision to include it at all?

@damiendoligez
Copy link
Member

@dra27 do you have the data on how much breakage we can expect from this PR ?

@dra27
Copy link
Member Author

dra27 commented Feb 11, 2020

Not at this stage, no - my guess it will be becoming a lower-impact patch, since Dune and friends do things properly (i.e. make is being used less)

@dra27 dra27 closed this Nov 25, 2020
quartz55 pushed a commit to quartz55/ocaml that referenced this pull request Feb 1, 2022
stedolan added a commit to stedolan/ocaml that referenced this pull request May 24, 2022
lpw25 pushed a commit to lpw25/ocaml that referenced this pull request Jun 21, 2022
stedolan pushed a commit to stedolan/ocaml that referenced this pull request Sep 21, 2022
454150b flambda-backend: Speed up testsuite (ocaml#658)
8362f9e flambda-backend: Speed up builds (ocaml#585)
a527cab flambda-backend: Update backends for changes from ocaml-jst

git-subtree-dir: ocaml
git-subtree-split: 454150b
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
* Add optional publication date to job records. Display it on the page. Sort offered positions, fresh first.
* Tried to add job publication to previously published jobs
* Removed closed position

Co-authored-by: Cuihtlauac ALVARADO <cuihtmlauac@tarides.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants