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

Delete otherlib/{graph,win32graph} #2318

Merged
merged 2 commits into from Mar 18, 2019

Conversation

@diml
Copy link
Member

commented Mar 12, 2019

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.

@diml diml force-pushed the diml:remove-graphics branch from bbc78f1 to 7dd414c Mar 12, 2019
@nojb

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

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

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

This comment has been minimized.

Copy link
@nojb

nojb Mar 12, 2019

Contributor

This part of the diff seems unrelated to the PR.

This comment has been minimized.

Copy link
@diml

diml Mar 13, 2019

Author Member

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

@diml diml force-pushed the diml:remove-graphics branch from 7dd414c to 275d02c Mar 13, 2019
@diml

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

Indeed. I removed it along with win32graph

@nojb

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

Indeed. I removed it along with win32graph

Ah, sorry I had missed that.

@diml

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

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.

@diml

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

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

Copy link
Contributor

left a comment

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.

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

This comment has been minimized.

Copy link
@xavierleroy

xavierleroy Mar 13, 2019

Contributor

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.

This comment has been minimized.

Copy link
@Octachron

Octachron Mar 13, 2019

Contributor

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

This comment has been minimized.

Copy link
@diml

diml Mar 14, 2019

Author Member

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

@diml diml force-pushed the diml:remove-graphics branch from 275d02c to cbb7f03 Mar 14, 2019
@diml

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

You are welcome :)

I updated the manual

@diml diml force-pushed the diml:remove-graphics branch from cbb7f03 to cc2a173 Mar 14, 2019
@shindere

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

@diml diml force-pushed the diml:remove-graphics branch from cc2a173 to 9ba7e0f Mar 18, 2019
@diml

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

@diml

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2019

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

@shindere

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

@diml

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2019

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.

@diml

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2019

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

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

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

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

"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

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

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

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

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>
@diml diml force-pushed the diml:remove-graphics branch from 9ba7e0f to 0725a7e Mar 18, 2019
@diml

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2019

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...

Copy link
Contributor

left a comment

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

@xavierleroy xavierleroy merged commit c413136 into ocaml:trunk Mar 18, 2019
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@shindere

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

@shindere

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

@shindere

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

dra27 added a commit to dra27/ocaml that referenced this pull request Apr 24, 2019
dra27 added a commit to dra27/ocaml that referenced this pull request Apr 24, 2019
shindere added a commit that referenced this pull request May 14, 2019
* 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*
smuenzel added a commit to smuenzel/ocaml that referenced this pull request Jun 26, 2019
* 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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.