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

Disable alpine build by default #2638

Merged
merged 3 commits into from
Jan 25, 2022
Merged

Conversation

jneira
Copy link
Member

@jneira jneira commented Jan 24, 2022

Unexpected usage error
Dynamic loading not supported

imagen

@pepeiborra
Copy link
Collaborator

Uh, good catch. We really should have dogfooded the Musl binaries.

But I don't understand what you want to release. We know that statically linking glibc is not a good idea, so let's investigate a bit more before reverting to that.

  1. Could the problem have been caught earlier by running the test suite in the build.yml CI script?
  2. Are all ghc versions affected ?
  3. Are the gitlab binaries affected?

@wz1000
Copy link
Collaborator

wz1000 commented Jan 25, 2022

I'm not sure why the alpine binaries don't work, I will investigate. The alternative for now is to do what I suggested in #2431 and compile against glibc, but don't statically link it in. This can be done by using executable-dynamic: False instead of executable-static: True. We can then do what GHC does and distribute a different binary for each supported distro/glibc version.

cabal-ghc921.project Outdated Show resolved Hide resolved
@jneira
Copy link
Member Author

jneira commented Jan 25, 2022

But I don't understand what you want to release. We know that statically linking glibc is not a good idea, so let's investigate a bit more before reverting to that.

I want to release the actual binary flavour as it will not break more use cases than the previous one. And i would prefer to make the release within January if possible to at least keep it bimonthly so if investigate and fix the problem will take more time we could do it after the release (and make another one before two months with the new binary)

1. Could the problem have been caught earlier by running the test suite in the build.yml CI script?

Sure, i expressed my concerns about when it was added and opened #2617 to restore consistence in ci using the same build in all workflows (including benchmarks)

2. Are all ghc versions affected ?

@kokobd and me tested 8.10.7, the default in ghcup and more used one, but we could test other ones

3. Are the gitlab binaries affected?

i did not test them neither, there is no user report here or in the ghcup issue tracker afaik, maybe @hasufell knows

@jneira
Copy link
Member Author

jneira commented Jan 25, 2022

I'm not sure why the alpine binaries don't work, I will investigate. The alternative for now is to do what I suggested in #2431 and compile against glibc, but don't statically link it in. This can be done by using executable-dynamic: False instead of executable-static: True. We can then do what GHC does and distribute a different binary for each supported distro/glibc version.

That alternative could be tested easily changing the rest of workflows. But our distribution channels other than direct download (vscode auto download and ghcup) doesn not take in account the linux distro/version so they should be changed (and that will take more time)

@jneira
Copy link
Member Author

jneira commented Jan 25, 2022

Will try to adress #2617 in another pr to test the alpine binary in our ci (but i guess it will break catastrophically the test suite)

@wz1000
Copy link
Collaborator

wz1000 commented Jan 25, 2022

But our distribution channels other than direct download (vscode auto download and ghcup) doesn not take in account the linux distro/version so they should be changed (and that will take more time)

GHCup can easily take distros into account when we add a metadata file for the next HLS release: https://github.com/haskell/ghcup-metadata/blob/master/ghcup-0.0.6.yaml#L2698

Instead of adding a single entry for Linux_UnknownLinux, multiple entries will need to be added for each distro version, as they already are for GHC.

@jneira
Copy link
Member Author

jneira commented Jan 25, 2022

GHCup can easily take distros into account when we add a metadata file for the next HLS release: https://github.com/haskell/ghcup-metadata/blob/master/ghcup-0.0.6.yaml#L2698

yeah, it already takes in account for ghc for necessity, but i will make the hls specific config more complex and the release harder as it is done semimanually
and changes in vscode will be everything but trivial

@wz1000
Copy link
Collaborator

wz1000 commented Jan 25, 2022

changes in vscode will be everything but trivial

I think we can re-use GHCup to download and install HLS in vscode, installing it(both GHCup and HLS) locally in the extension storage if it does not exist. I will try to write a patch to do this.

@jneira
Copy link
Member Author

jneira commented Jan 25, 2022

changes in vscode will be everything but trivial

I think we can re-use GHCup to download and install HLS in vscode, installing it(both GHCup and HLS) locally in the extension storage if it does not exist. I will try to write a patch to do this.

We have an issue in the repo about: haskell/vscode-haskell#483
It does not talk about download hls itself though. Add a new piece to the puzzle could bring tricky interactions (and add another point of failure), we should do it carefully.

Anyways it would delay the incoming release so maybe we should wait to 1.7 to do the adaptation

@wz1000
Copy link
Collaborator

wz1000 commented Jan 25, 2022

OK, for this release what if we distribute generic "linux" binaries statically linked with glibc(which will be used by the extension) and also distribute distro specific binaries that dynamically link against glibc which can be used by ghcup and also directly downloaded from the release page?

@pepeiborra
Copy link
Collaborator

Sure, i expressed my concerns about when it was added and opened #2617 to restore consistence in ci using the same build in all workflows (including benchmarks)

This is not enough, we need to run the testsuite in the build.yml workflow. But that can be added later, once we've sorted the alpine problem

@pepeiborra
Copy link
Collaborator

OK, for this release what if we distribute generic "linux" binaries statically linked with glibc(which will be used by the extension) and also distribute distro specific binaries that dynamically link against glibc which can be used by ghcup and also directly downloaded from the release page?

While I don't like the idea of having two flavours of binaries to support, it can be done if we add another user warning to highlight the statically linked glibc and document it, as in

displayTHWarning :: LspT c IO ()
displayTHWarning
| isMac && not hostIsDynamic = do
LSP.sendNotification SWindowShowMessage $
ShowMessageParams MtInfo $ T.unwords
[ "This HLS binary does not support Template Haskell."
, "Follow the [instructions](" <> templateHaskellInstructions <> ")"
, "to build an HLS binary with support for Template Haskell."
]
| otherwise = return ()

@wz1000
Copy link
Collaborator

wz1000 commented Jan 25, 2022

The dlopen failure "Dynamic loading not supported" happens because existing GHC alpine distributions are not statically linked, so GHC still tries to call dlopen. This should be fixed in future GHC releases by https://gitlab.haskell.org/ghc/ghc/-/merge_requests/5147

@pepeiborra
Copy link
Collaborator

I'm not sure why the alpine binaries don't work, I will investigate.

How does the GHC linker/loader detect that MUsl is being used, and is statically linked? It would need to in order to avoid calling dlopen.

@wz1000
Copy link
Collaborator

wz1000 commented Jan 25, 2022

I'm not sure why the alpine binaries don't work, I will investigate.

How does the GHC linker/loader detect that MUsl is being used, and is statically linked? It would need to in order to avoid calling dlopen.

It depends on the dynamic-system-linker cabal flag when compiling GHC, which is unfortunately not unset in any of the existing GHC distributions. It will be set properly in future alpine releases.

Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

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

Approving since it seems clear that we need to go back to glibc until GHC is set up to detect a static musl binary.

@jneira
Copy link
Member Author

jneira commented Jan 25, 2022

The dlopen failure "Dynamic loading not supported" happens because existing GHC alpine distributions are not statically linked, so GHC still tries to call dlopen. This should be fixed in future GHC releases by https://gitlab.haskell.org/ghc/ghc/-/merge_requests/5147

well ghc-9.2.1 was statically linked in alpine, no? and it produced problems in the ghc ci and bug like the described one in #2615 (comment) (ghc ticket https://gitlab.haskell.org/ghc/ghc/-/issues/20977) so @hasufell changed the official ghc bindist for alpine+9.2.1 with a dynamic (or non static? i am a bit lost) ghc in ghcup

@jneira
Copy link
Member Author

jneira commented Jan 25, 2022

Sure, i expressed my concerns about when it was added and opened #2617 to restore consistence in ci using the same build in all workflows (including benchmarks)

This is not enough, we need to run the testsuite in the build.yml workflow. But that can be added later, once we've sorted the alpine problem

given the actual master branch protection rules, there is no untested commit in master so it would serve to test a release made from a commit out of master, a very bad idea in its own. In exchange ci would be more complex, needing copy the entire test workflow or make use of reused workflows

@jneira
Copy link
Member Author

jneira commented Jan 25, 2022

Want to note that it is possible to trigger manual runs of the build workflow setting the alpine parameter to true

imagen

@jneira jneira merged commit 65dff92 into haskell:master Jan 25, 2022
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