Delete otherlib/{graph,win32graph} #2318

ghost · 2019-03-12T18:06:11Z

This PR deletes the graphics library. The plan is to distribute it as a separate package starting from OCaml 4.09.

I prepared a separate repository to be new home of the graphics library: https://github.com/ocaml/graphics. The git history was preserved in the process. The package is fully functional and ready to be released once 4.09 is released.

Users of the graphics library won't be affected by this change as long as they rely on the "graphics" findlib package. I checked all the users of graphics in opam, and found only one that will be affected by this PR: ocapic. Ocapic will need to be updated in order to be compatible with 4.09.

nojb · 2019-03-12T18:12:44Z

I think you need to update the variable otherlibs_all in the main Makefile as well.

nojb · 2019-03-12T18:14:37Z

configure

+  | --runstate=* | --runstat=* | --runsta=* | --runst=* | --runs=* \
+  | --run=* | --ru=* | --r=*)
+    runstatedir=$ac_optarg ;;
+


This part of the diff seems unrelated to the PR.

Agreed. However, I didn't add it. I simply ran autoconf which added this stuff. Should I delete it manually? /cc @shindere

ghost · 2019-03-13T08:45:50Z

Indeed. I removed it along with win32graph

nojb · 2019-03-13T10:04:06Z

Indeed. I removed it along with win32graph

Ah, sorry I had missed that.

ghost · 2019-03-13T14:34:34Z

I went over the reverse dependency of graphics and tried to build them against the external package (github.com/ocaml/graphics). Most of them compiled fine, I only had issues with the following packages:

camlimages: the build was broken but the fix was a one-liner. I sent a patch to the maintainer of camlimages
ocapic: fixing the build is a bit more complicated, I opened a ticket to discuss possible solutions
cairo2: I couldn't try as I'm having trouble installing the C library on my machine

All the other packages compiled just fine with the external package.

So even though it is not a completely transparent transition, it looks like it is not going to cause too much trouble in practice.

ghost · 2019-03-13T14:36:05Z

BTW, cairo2 is using dune so either it is not affected, either the fix is trivial as for camlimages.

xavierleroy

This pull request makes me all warm inside :-) Thank you @diml.

It looks very good to me. @shindere should tell us if the generated configure is OK or if it needs to regenerated. The manual could use some more help too.

xavierleroy · 2019-03-13T17:54:35Z

manual/manual/library/libgraph.etex

-\else
-\input{Graphics.tex}
-\fi
+https://github.com/ocaml/graphics


The LaTeX markup could use some more care, e.g. format the URL better and make it clickable. Perhaps @Octachron could help? Also, it would be good to mention the OPAM package.

\url{https://github.com/ocaml/graphics} should make the link clickable.

I updated the url and added a paragraph about the opam package

ghost · 2019-03-14T08:04:16Z

You are welcome :)

I updated the manual

shindere · 2019-03-15T12:48:41Z

Many thanks for externalizing this. A few comments: - The Change entry (or the commit message, but rather the change entry) could include the URL of the new source repository as is done in the error message in configure - You may want to add the names of the reviewers in the Change entry - Since Makefile.config gets installed, is it okay to just remove the GRAPHLIB variable?

ghost · 2019-03-18T10:24:34Z

I updated the changelog entry. Not sure what can be done for the GRAPHLIB variable. We could set it do a dummy value that we know will break. @dra27 what do you think, is it better to just remove it or set it to a dummy value such as the_graphics_library_is_no_longer_distributed_with_ocaml?

BTW, I'm a bit lost regarding what's happening with the configure file: initially, an extra block about runstate was generated. Then On Thursday I rebased my branch, ran autoconf again the extra block was no longer generated: all good. Now, this morning I rebased again my branch, ran autoconf again and the diff shows that the runstate block is being removed by my PR (???). I'm guessing this is related to the version of autoconf being used by the person generating the configure script...

shindere · 2019-03-18T12:03:30Z

Yeah, I think it has to do with the autoconf version used to generate the script. Perhaps, since we don't really need this block we could make sure this does not get committed. At the same time I'd say it does not really hurt. Also, to me that's the problem you gen when storing generated files in the repo. I'm not saying `configure` shouldn't be distributed in the tarballs of the compiler, quite the contrary actually, I do think it should be distributed. But I think it'd just be better to not have it in the repo, or at least not on trunk. It oculd be added e.g. to the release branches.

ghost · 2019-03-18T12:07:02Z

That seems right to me as well. I have always been allergic to committing such files in repositories.

shindere · 2019-03-18T12:26:45Z

Jérémie Dimino (2019/03/18 05:07 -0700):

That seems right to me as well. I have always been allergic to committing such files in repositories.

OK shall I submit a PR that removes configure from trunk then? At least so that this can be discussed? If I do so I'd like to Cc you @diml if you don't mind.

dra27 · 2019-03-18T12:36:39Z

For autoconf - the runstate thing is because there is a very old patch which is used by virtually all distros (and I think is committed to autoconf) but which has never been released. It usually disappears when you run autoconf on Cygwin and, given their usual philosophies, on the BSDs as well, I think.

For not committing configure - I have in mind to sort a patch which both declares configure as binary (which stops it from being included in diff views) and also add a test on Travis to ensure that configure.ac generates configure as given in the commit. I may manage to get to that this week, who knows!

configure tends to be an irritating special case for committing - I agree that it shouldn't need to be - though it is convenient not to require autotools in order to build a Git checkout of OCaml.

shindere · 2019-03-18T12:41:40Z

Well, to me the travis test and all this sounds a bit overkill, I have to say. I would way more prefer to not have configure at all in the repo. I feel it would be both easier and less fragile to addapt to this than to declare it binary,make sure it corresponds to configure.ac (which will require making sure the two versions of autoconf on the dev's box and on Travis generate the smae file.)

ghost · 2019-03-18T13:14:43Z

Yh, from the contributor's point of view, it's much easier if the configure script is not committed. Many people are not root on their machine at work so installing a particular version of autoconf might be a lot of hassle.

I vote for the configure script to not be committed. @shindere happy to be cced.

ghost · 2019-03-18T13:15:18Z

BTW, from my point of view this PR is ready to be merged, just waiting for approval :)

alainfrisch · 2019-03-18T13:19:35Z

Many people are not root on their machine at work so installing a particular version of autoconf might be a lot of hassle.

Are you making the assumption that one good-enough version of autoconf is readily available, for all developers and people that want to compile non-released code? With a committed configure, perhaps it's a more tedious to contribute for people who need to touch the configure script, but it's easier for all other users who simply want to compile or contribute to other parts.

alainfrisch · 2019-03-18T13:21:12Z

Also, I'm concerned that people could start using autoconf features which are not widely available. We might need to document which version is expected, not only for maintainers, but for everyone who will compile the code from Git.

dra27 · 2019-03-18T13:36:54Z

No - "virtually everyone" uses autoconf 2.69 with the runstate patch. autoconf 2.69 was released in 2012.

dra27 · 2019-03-18T13:38:06Z

@diml - your preference is contradictory. If configure is not committed then all contributors need autoconf because you then can't build OCaml without it?

xavierleroy · 2019-03-18T13:42:02Z

"virtually everyone" uses autoconf 2.69 with the runstate patch.

And Debian uses autoconf 2.69 without the patch, correct?

If that's true, I don't think we have much of a problem: when reviewing changes that affect configure.ac, either we don't look at the changes in generated configure, or we have a quick look knowing that the runstate stuff is unimportant.

For the record, I prefer to have configure committed in trunk and in release branches. Among other reasons, it's good to have exactly the same build procedures for trunk and release branches.

dra27 · 2019-03-18T13:44:02Z

Ubuntu uses it with the patch, so I'd assumed Debian does - I've not checked, though.

dra27 · 2019-03-18T13:45:03Z

Debian does have - it patch 6 in https://sources.debian.org/src/autoconf/2.69-10/debian/patches/series/

xavierleroy · 2019-03-18T13:51:35Z

OK, I read the diff the wrong way. So, for some reason the autoconf used by @diml doesn't have the runstate patch, or did not generate the runstate block. But "virtually everyone" should not run into this issue. For this reason I propose that:

@diml re-runs autoconf on various machines until the generated configure looks right;
everyone else stops worrying about committing configure.

This library is now distributed as a separate package. Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

ghost · 2019-03-18T13:56:13Z

Ok, so the autoconf on my laptop does generate the runstate block. I regenerated it and pushed. I was actually the one who removed the runstate block in the PR to remove vmthreads...

xavierleroy

Looks good, thanks. I fixed a documentation typo myself. Will merge next.

shindere · 2019-03-20T09:29:41Z

Alain Frisch (2019/03/18 06:21 -0700):

Also, I'm concerned that people could start using autoconf features which are not widely available. We might need to document which version is expected, not only for maintainers, but for everyone who will compile the code from Git.

Should we want to do such a thing, we could use the `AC_PREREQ` macro to enforce (rather than just document) an autoconf minimal version to be used.

shindere · 2019-03-20T09:32:41Z

Xavier Leroy (2019/03/18 13:42 +0000):

For the record, I prefer to have configure committed in trunk and in release branches. Among other reasons, it's good to have exactly the same build procedures for trunk and release branches.

Well then I'd prefer to have configure commited nowhere and just be part of the tarballs, but I understand this is not going to happen so I'm writing htis just for the record as well.

shindere · 2019-03-20T09:33:53Z

Xavier Leroy (2019/03/18 06:51 -0700):

OK, I read the diff the wrong way. So, for some reason the autoconf used by @diml doesn't have the runstate patch, or did not generate the runstate block. But "virtually everyone" should not run into this issue. For this reason I propose that: * @diml re-runs autoconf on various machines until the generated configure looks right; * everyone else stops worrying about committing configure.

Well commiting does create worries, whether we like it or not! ;-)

win32graph removed in ocaml#2318

* Make s-nt.h and m-nt.h more autoconf-like * Add SIZEOF_LONGLONG to m-nt.h It's not used, but autoconf will generate it. * Sections of s-nt.h and m-nt moved to config.h * Re-order lines of s-nt.h Allows the output to be more readily compared with autoconf's generated s.h. Lines are purely reordered so: diff <(git cat-file --textconv HEAD~1:config/s-nt.h | sort) \ <(git cat-file --textconv HEAD:config/s-nt.h | sort) should return no differences. * Add extra defines to s-nt.h for mingw32 These match up with facts determined by autoconf. The following additional HAS_ defines are made: - HAS_UNISTD Behaviour of otherlibs/unix/access.c and otherlibs/win32unix/lseek.c is affected but the effect is correct. Behaviour of runtime/fix_code.c affected, but unistd.h includes io.h, so there's no effective change. In other cases, unistd.h is simply included where before it wasn't. - HAS_DIRENT Used in Unix-only closedir.c, opendir.c, readdir.c and rewinddir.c Used in runtime/unix.c, but that obviously won't affect Windows! - HAS_REWINDDIR Used in Unix-only rewinddir.c - HAS_TRUNCATE Used in Unix-only ftruncate.c, mmap.c and truncate.c - HAS_NANOSLEEP Used in Unix-only sleep.c - HAS_GETTIMEOFDAY Used in vmthreads and Unix-only gettimeofday.c Used in runtime/sys.c, but in a branch guarded by #ifndef _WIN32 - HAS_MKSTEMP Changes yacc/main.c to use the MinGW implementation of mkstemp (part of mingwex, which is linked by flexdll) * Remove the old DISTRIB variable This variable was originally added in 1973b55 and 1596174 and was always specifically for Windows for installing the README files. This should have been renamed to INSTALL_DISTRIB in de4f4cf (which was part of MPR#6358/GPR#27). It should then have been removed in 13bb9d2 (GPR#1033). May it forever rest in peace. * Tweak the config/Makefile.m* to match autoconf * Build debug runtime by default on Windows * Build raw_spacetime_lib on msvc64 & mingw64 * Set ENABLE_CALL_COUNTS to true for Windows No-op, since spacetime is not enabled by default, but easier comparison with autoconf * Remove X11 definitions from old Makefiles win32graph removed in #2318 * mingw ports should use gcc for CPP * Correct MKEXEDEBUGFLAG for mingw ports -g is ignored by flexlink, -link -g is passed on to GCC. * Correct SO to be dll not s.obj/s.o Pedantic correction - $(SO) is not used by the Windows ports. * Re-order lines of config/Makefile.m*

* Make s-nt.h and m-nt.h more autoconf-like * Add SIZEOF_LONGLONG to m-nt.h It's not used, but autoconf will generate it. * Sections of s-nt.h and m-nt moved to config.h * Re-order lines of s-nt.h Allows the output to be more readily compared with autoconf's generated s.h. Lines are purely reordered so: diff <(git cat-file --textconv HEAD~1:config/s-nt.h | sort) \ <(git cat-file --textconv HEAD:config/s-nt.h | sort) should return no differences. * Add extra defines to s-nt.h for mingw32 These match up with facts determined by autoconf. The following additional HAS_ defines are made: - HAS_UNISTD Behaviour of otherlibs/unix/access.c and otherlibs/win32unix/lseek.c is affected but the effect is correct. Behaviour of runtime/fix_code.c affected, but unistd.h includes io.h, so there's no effective change. In other cases, unistd.h is simply included where before it wasn't. - HAS_DIRENT Used in Unix-only closedir.c, opendir.c, readdir.c and rewinddir.c Used in runtime/unix.c, but that obviously won't affect Windows! - HAS_REWINDDIR Used in Unix-only rewinddir.c - HAS_TRUNCATE Used in Unix-only ftruncate.c, mmap.c and truncate.c - HAS_NANOSLEEP Used in Unix-only sleep.c - HAS_GETTIMEOFDAY Used in vmthreads and Unix-only gettimeofday.c Used in runtime/sys.c, but in a branch guarded by #ifndef _WIN32 - HAS_MKSTEMP Changes yacc/main.c to use the MinGW implementation of mkstemp (part of mingwex, which is linked by flexdll) * Remove the old DISTRIB variable This variable was originally added in 1973b55 and 1596174 and was always specifically for Windows for installing the README files. This should have been renamed to INSTALL_DISTRIB in de4f4cf (which was part of MPR#6358/GPR#27). It should then have been removed in 13bb9d2 (GPR#1033). May it forever rest in peace. * Tweak the config/Makefile.m* to match autoconf * Build debug runtime by default on Windows * Build raw_spacetime_lib on msvc64 & mingw64 * Set ENABLE_CALL_COUNTS to true for Windows No-op, since spacetime is not enabled by default, but easier comparison with autoconf * Remove X11 definitions from old Makefiles win32graph removed in ocaml#2318 * mingw ports should use gcc for CPP * Correct MKEXEDEBUGFLAG for mingw ports -g is ignored by flexlink, -link -g is passed on to GCC. * Correct SO to be dll not s.obj/s.o Pedantic correction - $(SO) is not used by the Windows ports. * Re-order lines of config/Makefile.m*

nojb reviewed Mar 12, 2019

View reviewed changes

ghost mentioned this pull request Mar 13, 2019

graphics moving to an external package bvaugon/ocapic#4

Open

xavierleroy reviewed Mar 13, 2019

View reviewed changes

Delete otherlib/{graph,win32graph}

0725a7e

This library is now distributed as a separate package. Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

Fix typo in Graphics documentation

e7becdb

xavierleroy approved these changes Mar 18, 2019

View reviewed changes

xavierleroy merged commit c413136 into ocaml:trunk Mar 18, 2019

dra27 mentioned this pull request Mar 18, 2019

Tweaks to configure generation and testing #8517

Merged

stedolan mentioned this pull request Mar 20, 2019

Remove some unused configure tests. #8533

Merged

dra27 added a commit to dra27/ocaml that referenced this pull request Apr 24, 2019

Remove X11 definitions from old Makefiles

77ad1dc

win32graph removed in ocaml#2318

dra27 added a commit to dra27/ocaml that referenced this pull request Apr 24, 2019

Remove X11 definitions from old Makefiles

55d93e3

win32graph removed in ocaml#2318

copy mentioned this pull request Aug 17, 2019

Don't disable dynlink in -static switches ocaml/opam-repository#14664

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete otherlib/{graph,win32graph} #2318

Delete otherlib/{graph,win32graph} #2318

ghost commented Mar 12, 2019

nojb commented Mar 12, 2019

nojb Mar 12, 2019

ghost Mar 13, 2019

ghost commented Mar 13, 2019

nojb commented Mar 13, 2019

ghost commented Mar 13, 2019

ghost commented Mar 13, 2019

xavierleroy left a comment

xavierleroy Mar 13, 2019

Octachron Mar 13, 2019

ghost Mar 14, 2019

ghost commented Mar 14, 2019

shindere commented Mar 15, 2019 via email

ghost commented Mar 18, 2019

shindere commented Mar 18, 2019 via email

ghost commented Mar 18, 2019

shindere commented Mar 18, 2019 via email

dra27 commented Mar 18, 2019 •

edited

shindere commented Mar 18, 2019 via email

ghost commented Mar 18, 2019

ghost commented Mar 18, 2019

alainfrisch commented Mar 18, 2019

alainfrisch commented Mar 18, 2019

dra27 commented Mar 18, 2019

dra27 commented Mar 18, 2019

xavierleroy commented Mar 18, 2019

dra27 commented Mar 18, 2019

dra27 commented Mar 18, 2019

xavierleroy commented Mar 18, 2019

ghost commented Mar 18, 2019 •

edited by ghost

xavierleroy left a comment

shindere commented Mar 20, 2019 via email

shindere commented Mar 20, 2019 via email

shindere commented Mar 20, 2019 via email

Delete otherlib/{graph,win32graph} #2318

Delete otherlib/{graph,win32graph} #2318

Conversation

ghost commented Mar 12, 2019

nojb commented Mar 12, 2019

nojb Mar 12, 2019

Choose a reason for hiding this comment

ghost Mar 13, 2019

Choose a reason for hiding this comment

ghost commented Mar 13, 2019

nojb commented Mar 13, 2019

ghost commented Mar 13, 2019

ghost commented Mar 13, 2019

xavierleroy left a comment

Choose a reason for hiding this comment

xavierleroy Mar 13, 2019

Choose a reason for hiding this comment

Octachron Mar 13, 2019

Choose a reason for hiding this comment

ghost Mar 14, 2019

Choose a reason for hiding this comment

ghost commented Mar 14, 2019

shindere commented Mar 15, 2019 via email

ghost commented Mar 18, 2019

shindere commented Mar 18, 2019 via email

ghost commented Mar 18, 2019

shindere commented Mar 18, 2019 via email

dra27 commented Mar 18, 2019 • edited

shindere commented Mar 18, 2019 via email

ghost commented Mar 18, 2019

ghost commented Mar 18, 2019

alainfrisch commented Mar 18, 2019

alainfrisch commented Mar 18, 2019

dra27 commented Mar 18, 2019

dra27 commented Mar 18, 2019

xavierleroy commented Mar 18, 2019

dra27 commented Mar 18, 2019

dra27 commented Mar 18, 2019

xavierleroy commented Mar 18, 2019

ghost commented Mar 18, 2019 • edited by ghost

xavierleroy left a comment

Choose a reason for hiding this comment

shindere commented Mar 20, 2019 via email

shindere commented Mar 20, 2019 via email

shindere commented Mar 20, 2019 via email

dra27 commented Mar 18, 2019 •

edited

ghost commented Mar 18, 2019 •

edited by ghost