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

wrapper.in: Require runtime ghc-pkgs to be an abi compatible superset of bootpkgs #3214

Merged
merged 2 commits into from
Sep 28, 2022
Merged

wrapper.in: Require runtime ghc-pkgs to be an abi compatible superset of bootpkgs #3214

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c12379c
wrapper.in: Require runtime ghc-pkgs to be an abi compatible superset…
maralorn Sep 23, 2022
00edc7e
Merge branch 'master' into abi-wrapper-pkg-list
mergify[bot] Sep 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions GNUmakefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ bindist-ghc:
$(SED) \
-e "s/@@EXE_NAME@@/haskell-language-server-$(GHC_VERSION)/" \
-e "s/@@GHC_VERSION@@/$(GHC_VERSION)/" \
-e "s/@@BOOT_PKGS@@/$(shell ghc-pkg-$(GHC_VERSION) --global list --simple-output)/" \
-e "s/@@ABI_HASHES@@/$(shell for dep in `ghc-pkg-$(GHC_VERSION) --global list --simple-output` ; do printf "%s:" "$$dep" && ghc-pkg-$(GHC_VERSION) field $$dep abi --simple-output ; done | tr '\n' ' ' | xargs)/" \
bindist/wrapper.in > "$(BINDIST_OUT_DIR)/haskell-language-server-$(GHC_VERSION).in"
$(CHMOD_X) "$(BINDIST_OUT_DIR)/haskell-language-server-$(GHC_VERSION).in"
Expand Down
8 changes: 6 additions & 2 deletions bindist/wrapper.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
exedir="@@EXE_DIR@@"
executablename="@@EXE_NAME@@"
GHC_VERSION="@@GHC_VERSION@@"

# This space separated list contains the names and versions of the boot libraries used to compile hls.
BOOT_PKGS="@@BOOT_PKGS@@"
# This space separated list contains the ABI hashes of the pkgs in BOOT_PKGS at compiletime.
ABI_HASHES="@@ABI_HASHES@@"

debug_msg() {
Expand Down Expand Up @@ -62,7 +66,7 @@ check_ghc() {

# check version
if [ "${check_ghc_ver}" = "${GHC_VERSION}" ] ; then
# check ABI
# check for all packages listed in BOOT_PKGS that they are present with the same ABI hash as at hls-compiletime to prevent linking issues.
if "${GHC_PKG}" --version >/dev/null ; then
:
elif "${GHC_PKG}-${GHC_VERSION}" --version >/dev/null ; then
Expand All @@ -73,7 +77,7 @@ check_ghc() {
return 1
fi
PKGCONF="${check_ghc_libdir}/package.conf.d"
MY_ABI_HASHES="$(for dep in $("${GHC_PKG}" --global --global-package-db "$PKGCONF" list --simple-output) ; do printf "%s:" "${dep}" && "${GHC_PKG}" --global --global-package-db "$PKGCONF" field "${dep}" abi --simple-output ; done | tr '\n' ' ' | xargs)"
MY_ABI_HASHES="$(for dep in ${BOOT_PKGS} ; do printf "%s:" "${dep}" && "${GHC_PKG}" --global --global-package-db "$PKGCONF" field "${dep}" abi --simple-output ; done | tr '\n' ' ' | xargs)"
Copy link
Member

Choose a reason for hiding this comment

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

Where exactly do you allow a subset of packages here? You check everything that's used at compile time. Even if it's not relevant to HLS and if it's not an actual boot package. That's not a subset. It's either exactly all boot libraries or a superset of them.

I think a better idea is really to:

  1. hardcode the pkgs that are relevant to HLS ABI
  2. Only check those

I'd also be fine to hardcode the entire list of boot libraries. These may diverge for GHC versions in the future, but we only support a certain range anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am so, so, sorry. I had a stupid thinko/sign-error in the commit title. I meant all along, that the runtime packages can be a superset, not a subset of the compiletime/bootpkgs.

I think using the BOOT_PKGS variable in the wrapper script is the right thing and matching on all boot pkgs is also imo the right thing, because probably most packages are relevant to the HLS or TH ABI and figuring out which exactly aren’t could be annoying and probably not worth it. (At least that’s what Zubin argued for.)

Then we can argue about how to populate it. I think hardcoding it is simply more work, especially if we hardcode the versions, too. In all the cases (there are two) that I know where the wrapper is populated, the compiletime ghc-pkgs are exactly the boot pkgs, so I wouldn‘t worry about using that to populate the BOOT_PKGS variable …

Copy link
Member

@hasufell hasufell Sep 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I still don't follow. This will, imo, still fail if someone builds HLS in a non-nix env for a nix env, even if all boot ABIs match, because during runtime there are more packages in the global DB.

Same with cabal: if you install just one package into your global DB, ABI check will fail, even if all boot packages match.

That's why I'm arguing: this is a nix specific workaround.

Copy link
Member

Choose a reason for hiding this comment

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

I think I misunderstood the approach a little. I thought we still use the whole global DB during runtime.

Anyway... one could argue this still allows the ABI check to over-match, but building the bindists via the Makefile is not supposed to be done by the user. So it's up to the distributor to not mess it up.

As such, I think this is fine.

if [ "${ABI_HASHES}" != "${MY_ABI_HASHES}" ] ; then
err_abi "${MY_ABI_HASHES}"
return 3
Expand Down
1 change: 1 addition & 0 deletions haskell-language-server.cabal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ extra-source-files:
test/testdata/**/*.cabal
test/testdata/**/*.yaml
test/testdata/**/*.hs
bindist/wrapper.in

flag pedantic
description: Enable -Werror
Expand Down