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.

Sign up for GitHub

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

Jump to bottom

Support GHC nightly #20

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Support GHC nightly #20

wants to merge 10 commits into from

Conversation

brandonchinn178
Copy link
Member

@brandonchinn178 brandonchinn178 commented Jul 3, 2023 •
edited
Loading

Resolves #19

andreasabel
andreasabel reviewed Jul 11, 2023
View reviewed changes
Copy link
Member

@andreasabel andreasabel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great work, thanks!

  • Please add a test case for this feature to the CI.

  • Is the bump to node 20 essential? I don't know what the best practice for actions is atm, whether node 16 is still recommended. If there are no strong reasons to move away from the default (which seems 16 still), I'd like to stick to it. The setup-node action does not know about 20, it seems from the docs, the newest there is 18: https://github.com/actions/setup-node

action.yml Outdated Show resolved Hide resolved
@brandonchinn178
Copy link
Member Author

brandonchinn178 commented Jul 12, 2023 •
edited
Loading

No, the bump to Node 20 isn't essential, I'll remove it. I think I was getting confused about why dist/index.js was making additional changes, but I found the actual issue. See commit message: 9e4f89a

As an aside, why does CI not check that dist/index.js is properly regenerated? I believe that would've caught this issue. People could always bypass pre-commit hooks locally, so CI should always revalidate whatever pre-commit hooks are running

@brandonchinn178 brandonchinn178 force-pushed the ghcup-nightly branch 2 times, most recently from 7f85bbc to 76cbb23 Compare July 12, 2023 03:45
andreasabel
andreasabel reviewed Jul 12, 2023
View reviewed changes
Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

Since I am not the architect of this action but just stepped in for a burned-out maintainer I don't know how the .js generation works and what could go wrong with it. I am happy to accept additions to CI that test that it is generated correctly.

src/installer.ts Outdated Show resolved Hide resolved
src/setup-haskell.ts Outdated Show resolved Hide resolved
src/setup-haskell.ts Outdated Show resolved Hide resolved
@brandonchinn178 brandonchinn178 force-pushed the ghcup-nightly branch 2 times, most recently from 0368e28 to a709f1e Compare July 13, 2023 04:54
@brandonchinn178
Copy link
Member Author

@andreasabel I pushed a tentative implementation, let me know what you think.

I'm also unsure why the windows job is failing now; it passed last time: https://github.com/haskell-actions/setup/actions/runs/5527334962/jobs/10082976242

@andreasabel
Copy link
Member

@andreasabel I pushed a tentative implementation, let me know what you think.

Great, thanks for the update!

On a cursory look, I noticed that the ghcup-release-channels is implemented as a comma-separated list. Is there a specific reason for this?
A priori, I would expect that a YAML list is used here.

I'm also unsure why the windows job is failing now; it passed last time: haskell-actions/setup/actions/runs/5527334962/jobs/10082976242

This could be an upstream problem. I noticed that in the failing run, you got an older version of the Windows runner than in the successful run:

In particular, in the older image ghcup additionally prints New ghc version available. If you want to install this latest version, run 'ghcup install ghc 9.6.2'. This indicates that the older image does not have the most recent GHC, so it could also have a version of ghcup that is too old (mere speculation).

@brandonchinn178
Copy link
Member Author

brandonchinn178 commented Jul 13, 2023 •
edited
Loading

A priori, I would expect that a YAML list is used here.

We can't use YAML lists 😢 Workflow inputs are converted into env vars. But actually, I do see @actions/core has a helper for parsing a multiline string input, so let me switch to that instead of a comma separated string

Also, I thought the action upgrades ghcup to the version in the versions file? Does it not for windows?

EDIT: oh I see it aborts before installing ghcup:

setup/src/installer.ts

Line 315 in 1ffbf9f

return 'ghcup';

It also seems like ghcup doesn't provide windows 32 binaries.

@brandonchinn178 brandonchinn178 force-pushed the ghcup-nightly branch 2 times, most recently from b95c795 to 8970902 Compare July 13, 2023 16:30
@brandonchinn178 brandonchinn178 mentioned this pull request Jul 14, 2023
Merged
@brandonchinn178 brandonchinn178 force-pushed the ghcup-nightly branch 4 times, most recently from 0c50b6d to 3f27154 Compare July 14, 2023 04:22
@andreasabel
Copy link
Member

@brandonchinn178 wrote:

But I could switch to a multiline string instead of a comma separated string if you want

Yes, I think that would be more idiomatic. I know it e.g. from actions/cache inputs path and restore-keys and many other actions.

Also, I thought the action upgrades ghcup to the version in the versions file? Does it not for windows?

I would expect the same. Apparently something is not working correctly here.
The code you cite originally comes from this PR, which added the ghcup install method to Windows:

@brandonchinn178
Copy link
Member Author

I tried removing the windows early exit, and it didnt work. ghcup has windows 64 binaries but github actions uses windows 32. similarly, i tried ghcup upgrade, but that also failed

@brandonchinn178
Copy link
Member Author

@andreasabel I saw you merged #27; what should I do with this PR? Should I break out the various refactorings?

  • Remove broken HEAD logic
  • Add new ghc-release-channels input
  • Typescript improvements

@andreasabel andreasabel mentioned this pull request Jan 25, 2024
Merged
@andreasabel andreasabel added this to the 3.0.0 milestone Jan 25, 2024
@ulysses4ever
Copy link
Contributor

Sad to see this lingering...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreasabel andreasabel Awaiting requested review from andreasabel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.

Fix HEAD and/or support GHC nightlies
3 participants
@brandonchinn178 @andreasabel @ulysses4ever