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

conf-time #9848

Merged
merged 2 commits into from
Jul 16, 2017
Merged

conf-time #9848

merged 2 commits into from
Jul 16, 2017

Conversation

gasche
Copy link
Member

@gasche gasche commented Jul 16, 2017

I got the idea from looking at #9832 which seems to fail finding /usr/bin/time on CentOS. This change does in itself solve this problem (the only depext moved around are the existing ones for debian and ubunutu), and I'm not very knowledgeable in conf-* packages (is which time a reasonable build command?), but this goes in the spirit of #5791.

bug-reports: "https://github.com/ocaml/opam-repository/issues"
dev-repo: "https://github.com/ocaml/opam-repository.git"
license: "GPL"
build: [["which" "time"]]
Copy link
Contributor

Choose a reason for hiding this comment

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

If you use which you should depend on conf-which.

build: [["which" "time"]]
depexts: [
[["debian"] ["time"]]
[["ubuntu"] ["time"]]
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to extend the depexts a bit for other platforms distributions, here's a list of system package manager online search that can help.

@gasche
Copy link
Member Author

gasche commented Jul 16, 2017

Thanks @dbuenzli! I added the dependency and will be looking over the package lists.

@gasche
Copy link
Member Author

gasche commented Jul 16, 2017

Here is a difficulty: most BSD or BSD-inspired system (OSX) use a different implementation of time that does not support the options that bitstring is relying on, but they provide gnu-time packages that install a gtime command. The bitstring configuration right now does not test for gtime as an alternative to time, but it does allow to pass the correct time command as a configuration variable. Is there an opam-1.2 way to transfer information from conf-time to user packages about the name of the GNU time command? I see three options:

  • maybe I can set an environment variable (the documentation seems to suggest that env: ["GNU_TIME_COMMAND_NAME" = "time"] should work, but env: seems to have never been used in the public repository. What's a good variable name?
  • maybe I can set an OPAM package variable, but if I understand correctly this only works under opam 2
  • maybe I can have the package install a time command in %{bin}%. What's a good command name? (time, gnu-time?)

If you know of a package that had to deal with this, suggestions are welcome.

P.S.: By far the easiest way forward to solve the "bitstring installation fails if GNU time is not there" would be to change bitstring to stop depending on GNU time. I may do this independently.

@dbuenzli
Copy link
Contributor

Is there an opam-1.2 way to transfer information from conf-time to user packages about the name of the GNU time command?

I think the simplest for now is to use an os filter on the configuration argument in the bitstring package (though this won't work for distribution specifics as I don't think there's a variable for them) e.g. :

"configure" "--time-bin" "time" {os = "linux"} 
                         "gtime" {os = "darwin"}

and which the appropriate binary according to the os in conf-time (yes it's not DRY).

maybe I can set an environment variable (the documentation seems to suggest that env: ["GNU_TIME_COMMAND_NAME" = "time"] should work, but env: seems to have never been used in the public repository. What's a good variable name?

I'm not sure this even works pre-opam v2. And there's a lot of things that are currently unclear or undocumented about this (e.g. how to guarantee a deterministic environment w.r.t. to a sequence of package installs).

@gasche
Copy link
Member Author

gasche commented Jul 16, 2017

Ok. Then I would suggest to consider that the semantics of conf-time is that a time command is available (GNU, BSD, Busybox etc.) -- I'll edit the descr to suggest this -- and to handle packages that require the GNU options specially.

@dbuenzli
Copy link
Contributor

Another alternative is to upstream support for a POSIX usage of time(1), if that would work.

gasche added 2 commits July 16, 2017 09:31
This package does *not* enforce availability of "GNU time", it may be
BSD time (BSD, OSX) or Busybox's (Alpine). The only common
functionality is "time <command>".
@gasche
Copy link
Member Author

gasche commented Jul 16, 2017

I found an example of conf-package-communication in the conf-llvm packages (see #9853), and it is quite neat. The build script for the package runs a configure script which generates a conf-llvm.config file that contains fields in opam syntax, and those are automatically added to the opam internal environment, so conf-llvm:version can then be used to query the current LLVM version.

So if we wanted a package that enforces GNU time instead of any other time command, it could be a conf-gnu-time package whose build script generates a conf-gnu-time.config file at build time containing a command variable that would be either time or gtime -- and then other packages could use conf-gnu-time:command. I think that in this specific case, fixing the upstream to stop depending on the GNU time command for installation (xguerin/bitstring#2) is way easier/better, but that's a neat trick to keep in mind for the next conf-* outbreak.

@gasche gasche mentioned this pull request Jul 16, 2017
@gasche
Copy link
Member Author

gasche commented Jul 16, 2017

Because the replacement of depexts by conf-time touches so many dependencies, the CI job is taking forever (my last push was 10 hours ago and CI is not done yet) and returns failures that are obviously due to other metadata issues (two bap versions fail because llvm is not correctly detected, which fortunately is something also on my radar for later). I reviewed the patch again to ensure that it is semantics-preserving for all affected packages, so I am thinking of going ahead and merging the PR. @dbuenzli, would you play the partner in crime by agreeing with the plan?

@dbuenzli
Copy link
Contributor

dbuenzli commented Jul 16, 2017

I had a look at the travis failures and some of the CI and they seem indeed unrelated to the change. I accept the partnership.

Regarding travis they concern bap 0.9.5 @ivg may be interested in having a look at them and PR better constraints to the repo.

@gasche gasche merged commit df1d0c5 into ocaml:master Jul 16, 2017
@gasche
Copy link
Member Author

gasche commented Jul 16, 2017

Thanks, merged. The bap failure that I have seen are different from those (they were config-time failures rather than build failures, interesting that we see both with different CIs), they are there and there -- in both case, grep for E: to get to the error line.

@avsm
Copy link
Member

avsm commented Jul 17, 2017

This looks fine to me. On the CI point, it's ok to use up resources on the (still in beta) datakit-based service, as it can be restarted easily. For base patches like this though, please try not to submit them on a Friday and expect a review over the weekend -- it's more reasonable to give reviewers the work week to try and respond, and save the weekend for fun patches :-)

@gasche
Copy link
Member Author

gasche commented Jul 17, 2017

Thanks for the process advice, noted.

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.

3 participants