actions / setup-haskell

Set up your GitHub Actions workflow with a specific version of Haskell (GHC and Cabal)
MIT License
71 stars 30 forks source link

Update GHC versions on windows #47

Closed thboop closed 4 years ago

thboop commented 4 years ago

So, this is a fairly lengthy pr to update GHC versions on windows.

Context

To begin with, we install GHC differently depending on OS. For windows, we use the tool Choco. Which adds GHC to path using the add-path command, which was just recently disabled. This causes the workflow run to fail.

So, working with GhcChoco, we made some new versions that no longer use the disabled command. However, these only exist on choco, not the other platforms. Previously, the version list of GHC for all os's was the same. So thats our first major change:

However, when we did that, we ran into some other issues which we have fixed:

Resolves #44

tclem commented 4 years ago

@jared-w if you've got a free moment, would appreciate your thoughts on this as well.

hazelweakly commented 4 years ago

@tclem yeah, I have a few thoughts, thanks for the mention.

There's a few subtle things happening here that make it to where I am unconvinced the PR needs to take the approach it currently does.

  1. When Tamar is talking about github action compatibility in the chocolatey scripts, that is only for adding the GHC binaries to the path. This is so that when chocolatey is used directly, it "just works". I have never relied on that behavior and have always added the path manually, myself.

    As such, the supported versions of GHC on windows do not change because it still works the same way it used to. What has changed is if one installs GHC 8.10.X, or 8.8.X, it will use the patch version which does not call the removed command any longer.

    But, setup-actions has never relied on that behavior. So it is sufficient to "resolve" a version like 8.8.4 to 8.8.4.1 on windows, and just add that path; unaffected versions do not need to change, so there doesn't need to be a split between windows and macos/linux version support of GHC.

  2. The version difference exists only because chocolatey doesn't allow revisions of uploaded versions (which is the correct thing to do). If someone manually installs GHC 8.4.4 through chocolatey, for example, it'll use the patch version automatically (as far as I can figure out). We should be explicit because it makes generating the path easier, but fundamentally we haven't semantically changed anything here.

  3. The timing of the add-path file vs the add-path environment file is a breaking change and should be addressed if it causes the action to break on Windows. The added path logic to isInstalled is indeed the best place to fix that, imo.

    I am a bit grumpy that this is effectively impossible to unit test, but that's outside of the scope of the PR.

In summary:

hazelweakly commented 4 years ago

And as a general note, while I don't myself use Windows on my personal machines, I see it as important to have it on equal footing whenever possible.

Even if it were necessary to change a lot of the internal reworkings of how we invoke chocolatey (or install GHC/cabal on windows in general) in order to accommodate other GHC versions and keep the version specification the same, I would be much more willing to do that; particularly since the most core API of setup-haskell is the ghc-version option. Absolutely every other bit of logic in the action is replaceable in semi-trivial amounts of code except for that.

Having a visible breaking change to how Windows users specify GHC versions solely due to github-action implementation implementation details is truly the worst of both worlds. Luckily, it doesn't seem to be necessary. This is not to diminish Thomas's work; chasing tooling breakages across several layers of abstraction and coming up with a working solution is no small feat.

thboop commented 4 years ago

Hey @jared-w thanks for the feedback! I've laid out what I think our options are below, but I'm open to other ideas! I would love to hear your thoughts!

If someone manually installs GHC 8.4.4 through chocolatey, for example, it'll use the patch version automatically (as far as I can figure out)

In my testing, this did not occur. For example the workflow

name: CI
on:
  push
jobs:
  test:
    runs-on: windows-latest
    steps:
     - run: "choco install ghc --version 8.10.2 -m --no-progress -r" # this will fail
       continue-on-error: true
     - run: "choco install ghc --version 8.10.2.2 -m --no-progress -r" # this works

The first step fails while the second works

When Tamar is talking about github action compatibility in the chocolatey scripts, that is only for adding the GHC binaries to the path. This is so that when chocolatey is used directly, it "just works". I have never relied on that behavior and have always added the path manually, myself.

This action appears to have relied on the functionality. Hence we had to add the change to core.addPath to manually add it to the path if choco does not.

We fail the step if we detect any usage of add-path so installing any of these older (non-patch) versions instantly fails on windows. This was done to give instant actionable feedback to users, rather than allowing the action to continue without setting the path which would have be a confusing, somewhat invisible error if they don't check the logs or annotations. So any user installing any of these non specific patch versions of ghc on windows will fail. Hence we separate out the windows list here.

The way I see it, we have 4 options:

4 is probably the best solution, but is out of our hands and likely would not be available for a while. 3 is what this pr implemented, but may cause minor breakage for users reliant on specific versions that have not been patched. 2 feels somewhat hacky to rely on, but if we could get it working would likely be the least disruption. 1 requires a coordinated rollout across a lot of systems, which can be slow.

Mistuke commented 4 years ago

Hi folks,

just my 2 cents..

Hey @jared-w thanks for the feedback! I've laid out what I think our options are below, but I'm open to other ideas! I would love to hear your thoughts!

If someone manually installs GHC 8.4.4 through chocolatey, for example, it'll use the patch version automatically (as far as I can >> figure out)

In my testing, this did not occur. For example the workflow

Correct, using a specific version resolves to exactly that version. There is an open feature request for this but it's not available yet https://github.com/chocolatey/choco/issues/800

So it is sufficient to "resolve" a version like 8.8.4 to 8.8.4.1 on windows

I think this is the right thing to do. As @jared-w said the point releases are just because chocolatey won't let me revise the packages. Now the point releases will never contain functionality changes wrt to the compiler itself. They are always just packaging related.

as such there will never be a functional difference, only a difference in what the installer does, since a change in functionality will be a bump in minor version from upstream not the revision number. For instance the 8.10.2.1 revision fixes the hardcoded linker issue with the upstream binary, without touching the package they provide. It just post-processes the settings file after install.

As such I think it makes more sense to ignore the revision number and just map them internally to the latest one. my intention with the revisions are that users shouldn't care about which number it is. This will allow you to treat Windows the same as Linux etc w.r.t version numbers.

thboop commented 4 years ago

Thank you everyone for jumping in here on such short notice.

Based on the feedback, I believe our goals should be:

We can accomplish that by:

I'll go ahead and update this pr and get this version released. Since this is a minor version with no breaking changes, if we decide on a solution that we feel is better in the future (that may take longer to implement), we can major version the action and spend the time to do that. However, I would like to get this fixed asap.

Thanks again folks!

hazelweakly commented 4 years ago

Disabling command execution is a good way to accomplish that, nice; I hadn't thought of it initially.

We should, however, eventually map all patch releases on chocolatey internally so that we use the "most correct" version of the installer for any given GHC version. But that can be in a follow up.

phadej commented 4 years ago

Do I interpret these changes right: I need to do

      - uses: actions/setup-haskell@v1.1.4
        with:
          ghc-version: '8.10.2.2' # 8.10.2 isn't automatically remapped to 8.10.2.2?
          cabal-version: '3.2.0.0'
thboop commented 4 years ago

Do I interpret these changes right: I need to do

      - uses: actions/setup-haskell@v1.1.4
        with:
          ghc-version: '8.10.2.2' # 8.10.2 isn't automatically remapped to 8.10.2.2?
          cabal-version: '3.2.0.0'

@phadej You should just use the existing versions

       - uses: actions/setup-haskell@v1.1.4
         with:
          ghc-version: '8.10.2
Mistuke commented 4 years ago

Do I interpret these changes right: I need to do

      - uses: actions/setup-haskell@v1.1.4
        with:
          ghc-version: '8.10.2.2' # 8.10.2 isn't automatically remapped to 8.10.2.2?
          cabal-version: '3.2.0.0'

@phadej You should just use the existing versions

       - uses: actions/setup-haskell@v1.1.4
         with:
          ghc-version: '8.10.2

Unless 8.10.2 is internally mapped to 8.10.2.1 or 8.10.2.2 he shouldn't though... as 8.10.2 is broken as released by GHC HQ.

hazelweakly commented 3 years ago

It will be internally mapped from 8.10.2 -> 8.10.2.2 but currently that hasn't been done yet. I'm hoping to write up that functionality and get it merged shortly so that 8.10.2 will "just work".

In the meantime, versions like 8.10.2.2 and other unrecognized versions will transparently pass through to the underlying tool. So 8.10.2.2 will work on windows but will break on linux.