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

new-freeze: write with-compiler line to freeze file #4071

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hvr
Copy link
Member

@hvr hvr commented Nov 1, 2016

In order for a freeze file generated by new-freeze to make sense
we need to record the compiler that was used. Moreover, this bypasses
the confusing error messages you'd get from the solver if the freeze
file isn't compatible with the currently selected compiler.

As it's tradition for compilers such as gcc or clang, GHC's
build-system installs version suffixed symlinks of the ghc executable,
so we can exploit that to document/record/freeze the compiler version.

In order for a freeze file generated by `new-freeze` to make sense
we need to record the compiler that was used. Moreover, this bypasses
the confusing error messages you'd get from the solver if the freeze
file isn't compatible with the currently selected compiler.

As it's tradition for compilers such as `gcc` or `clang`, GHC's
build-system installs version suffixed symlinks of the `ghc` executable,
so we can exploit that to document/record/freeze the compiler version.
@mention-bot
Copy link

@hvr, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dcoutts and @ezyang to be potential reviewers.

@23Skidoo
Copy link
Member

23Skidoo commented Nov 1, 2016

If I run freeze -w /path/to/ghc, will it record the path that I gave?

@ezyang ezyang added this to the 2.0 milestone Nov 1, 2016
@ezyang
Copy link
Contributor

ezyang commented Nov 1, 2016

No; it only records the compiler id which doesn't contain a path.

@ezyang
Copy link
Contributor

ezyang commented Nov 1, 2016

So, if I get a freeze file from someone and don't have the compiler in my path, it's going to complain "cabal: Cannot find the program 'ghc'. User-specified path 'ghc-7.10.3' does not refer to an executable..."? That doesn't seem so good?

Also, if I pass in a -w flag, won't that override the freeze flag and I'll get the old bad version?

It seems like you really want a proper "compiler-version" field rather than coopting the existing "with-compiler"

@hvr
Copy link
Member Author

hvr commented Nov 1, 2016

@ezyang well, what I actually want is for cabal to find the proper GHC version (i.e. the user shouldn't be required to redundantly specify the matching ghc executable if there is one reachable via $PATH). If instead we use a new compiler-version field, how is cabal going to find the respective ghc(js,vm) executable? Are we going to implement a new heuristic that e.g. tries ghc first, and if that ones not the requested version, tries for an executable ghc-7.10.3?

PS: Also, when the freeze file says compiler-version: ghc-7.10.3, and there is no ghc-7.10.3 somewhere it almost always means there is no GHC 7.10.3 available, so the freeze file can't be satisfied anyway. So an error is unavoidable.

@ezyang
Copy link
Contributor

ezyang commented Nov 1, 2016

I'm sympathetic to your goal, but the concerns I raised above still hold ;)

Won't things be better if we have per-GHC freeze files? In this case, you shouldn't end up in a situation where you try to use a freeze file for the wrong version of GHC.

@hvr
Copy link
Member Author

hvr commented Nov 2, 2016

We talked a bit about this on IRC, and it may indeed make sense to have a separate field from with-compiler to capture the CompilerId, let's call it needful-compiler-version as a placeholder name for now:

The semantics I'd like are based on the presence of with-compiler and needful-compiler-version:

  1. If there's an explicit with-compiler setting, use that compiler exe (and warn loudly if it's in conflict with needful-compiler-version)

  2. If there's no explicit with-compiler setting and there's no needful-compiler-version setting either, locate ghc as usual

  3. If there's a needful-compiler-version and no explicit with-compiler set,

    a. if the default-located ghc matches, use that one, if not
    b. try to locate a version suffixed ghc-x.y.z via PATH
    c. if no matching GHC can be located, fail (the user now has to specify one via with-compiler, which means go to step 1. )

NB: I don't suggest that the solver picks the compiler-version (even if it could) at this point


PS: There's also a compiler: property already (which corresponds to --ghcjs or --ghc and also changes what executable-name cabal looks for by default) which needs to be taken into account; so we can either

  1. extend compiler: to allow to take a version suffix optionally, e.g. compiler: ghcjs and compiler: ghcjs-8.0.1 (or maybe even compiler: ghcjs == 8.0.1 to allow for version ranges at some point in the future) would both be valid, or
  2. we could add a companion to compiler: named compiler-version: which takes only a version (NB: compiler defaults to ghc currently), e.g. compiler-version: 8.0.1 would refer to version 8.0.1 of whatever compiler: is currently set to.

Then compiler (+ compiler-version for variant 2) would represent the imaginary needful-compiler-version as outlined above in the first half of this comment.

@ezyang
Copy link
Contributor

ezyang commented Nov 2, 2016

I think this is an OK design, though a bit special casey (there's no way to control what GHCs needful-compiler-version rolls up besides manipulating the PATH). Definitely needs user manual explaining, and of course we need a better field name.

@23Skidoo 23Skidoo modified the milestones: 2.2, 2.0 Feb 17, 2017
@23Skidoo
Copy link
Member

Looks like this PR is not going to be merged in its current state, can we close it for now and open an issue about adding needful-compiler-version?

@ezyang
Copy link
Contributor

ezyang commented Feb 20, 2017

I've mostly resigned myself to the idea that we'll have some indefinitely open PRs in the Cabal repo :) I'll let @hvr decide what he wants to do here.

@alanz
Copy link
Collaborator

alanz commented Feb 20, 2017

Silly point, would frozen-with-compiler not be a better name rather than needful-compiler-version. It indicates that it is known to work with that compiler, but ymmv with others.

@23Skidoo
Copy link
Member

@alanz needful-compiler-version is a placeholder name, frozen-with-compiler does sound better.

@23Skidoo 23Skidoo modified the milestones: 2.2, 2.4 Aug 29, 2018
@23Skidoo 23Skidoo modified the milestones: 2.4, 2.4.1 Sep 17, 2018
@23Skidoo 23Skidoo modified the milestones: 2.4.1.0, 2.4.2.0 Apr 26, 2019
@phadej phadej modified the milestones: 2.4.2.0, 3.4 Nov 27, 2019
@hasufell
Copy link
Member

hasufell commented Jan 9, 2020

I believe this feature should be merged as-is, possibly behind a cli flag if people deem that necessary. It can be improved later.

@Mikolaj
Copy link
Member

Mikolaj commented May 26, 2021

@hasufell: would you make that happen? I'd help. I guess, at least we should rebase, address old review comments as much as feasible and review again.

@hasufell
Copy link
Member

I might have a look at it. I'm currently neck-deep in ghcup-windows support stuff. Is there a schedule for the next release?

@Mikolaj
Copy link
Member

Mikolaj commented May 26, 2021

AFAIK the scope of the next release is mostly closed, unless there is a good reason to squeeze something in, so no hurry, unless you'd personally need it ASAP. As far as I'm concerned, I'd just hate to see lots of good brainstorming and motivation coming from user experience go to waste, hence prodding. :)

@Mikolaj
Copy link
Member

Mikolaj commented Nov 30, 2021

Anybody willing to give this PR a prod? Or comment?

@gbaz
Copy link
Collaborator

gbaz commented Feb 24, 2022

I think the objections to merging this approach still hold, and don't think it makes sense to merge as is.

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 1, 2022
@ulysses4ever ulysses4ever removed the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 3, 2022
@Kleidukos
Copy link
Member

Marking this PR as draft 🙂

@Kleidukos Kleidukos marked this pull request as draft May 17, 2023 06:26
@andreabedini
Copy link
Collaborator

#9021 is related to this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.