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

Check for ncurses6 before checking for tinfo6 #3521

Closed
wants to merge 1 commit into from
Closed

Check for ncurses6 before checking for tinfo6 #3521

wants to merge 1 commit into from

Conversation

dtaskoff
Copy link

@dtaskoff dtaskoff commented Oct 27, 2017

On Arch Linux which has both libraries, the tinfo6 distribution will be picked, which doesn't work.

I'm not sure how to test if this is breaking anything, so I'll gladly accept some guidance.

Please include the following checklist in your PR:

  • Any changes that could be relevant to users have been recorded in the ChangeLog.md
  • The documentation has been updated, if necessary.

On Arch Linux which has both libraries, the tinfo6 distribution will be picked, which doesn't work.
@mgsloan
Copy link
Contributor

mgsloan commented Oct 27, 2017

Seems reasonable, I'm in favor of merging, and seeing if anyone complains about the change in behavior

@borsboom What do you think?

@borsboom
Copy link
Contributor

Testing this on a few other distros would be a good idea. There was a similar change in #3343 (for Gentoo) so should make sure this doesn't break it (ping @rdnetto) and that PR was also tested on Ubuntu LTS, the latest Ubuntu, and Fedora.

@rdnetto
Copy link
Contributor

rdnetto commented Oct 28, 2017

Running it under Sabayon results in the architecture linux64-ncurses6-nopie, as all three versions are present:

# ldconfig -p | egrep 'lib64.*(ncursesw.so.6|libtinfo.so.[56])'
        libtinfo.so.6 (libc6,x86-64) => /lib64/libtinfo.so.6
        libtinfo.so.5 (libc6,x86-64) => /usr/lib64/libtinfo.so.5
        libncursesw.so.6 (libc6,x86-64) => /lib64/libncursesw.so.6

This breaks under Sabayon, because the linux64-ncurses6-nopie architecture* doesn't include the -fno-PIE flags present in linux64-tinfo6-nopie. We could add the flags, but there's the risk of breaking existing users of that architecture. (Although I'm not sure how much sense it makes for a -nopie architecture to not have them enabled.)

(Similarly, it looks like under Fedora the architecture will change from linux64-tinfo6 -> linux64-ncurses6.)

There's also the separate problem that linux64-ncurses6-nopie only has a subset of the GHC versions that linux64-tinfo6-nopie does, though that's easily fixed.

I'm curious why the tinfo6 architecture doesn't work under Arch if it's being detected - is it the same problem as with Sabayon, where there are implicit dependencies?

If we can change the flags for linux64-ncurses6-nopie to match those for linux64-tinfo6-nopie, that's probably the simplest and easiest solution. Otherwise, it might be time for us to bite the bullet and stop encoding the compiler flags in the architecture name...

* This is more accurately the 'GHC build', but I wrote most of this before I remembered the correct term for it.

@dtaskoff
Copy link
Author

Curiously, after running the sed command suggested in #3518, but replacing with 8.0.2 (the version I'm using), the linux64-tinfo6-nopie GHC works on Arch Linux.
So, is it just some problem with the bindist?

@rdnetto
Copy link
Contributor

rdnetto commented Nov 1, 2017

Sed command for reference:

sed -i 's/-fno-PIE/-no-pie/g' ~/.stack/programs/x86_64-linux/ghc-tinfo6-nopie-8.2.1/lib/ghc-8.2.1/settings

Relevant part of the file:

 ("C compiler command", "/usr/bin/gcc"),
 ("C compiler flags", "-fno-PIE -fno-stack-protector"),
 ("C compiler link flags", ""),
 ("C compiler supports -no-pie", "NO"),
 ("Haskell CPP command","/usr/bin/gcc"),
 ("Haskell CPP flags","-E -undef -traditional"),
 ("ld command", "/usr/bin/ld"),
 ("ld flags", "-no-pie"),

If I try to apply that change to ghc-tinfo6-nopie-8.0.2 on Sabayon and compile a trivial hello world program, I get:

gcc: error: unrecognized command line option ‘-no-pie’
`gcc' failed in phase `C Compiler'. (Exit code: 1)

This is probably because Sabayon uses GCC 4.9.3, and the flag was added in GCC 6. The content appears to be generated, as it's not present in the tarball but there are similarly named .mk files. Grepping the contents of the GHC tarball suggests the generation is performed by ./configure, and the values are determined by environment variables such as $CONF_CC_OPTS_STAGE2, which are defined in stack-setup-2.yaml in terms of the build architecture.

This is fundamentally broken - the flags we want to specify depend on the version and configuration of GCC installed on the user's system, but we don't have the ability to express this via stack-setup-2.yaml. For added fun, those properties can change whenever the user upgrades their distro, so the existing approach of generating the file once and expecting it to continue working is also broken.

It's increasingly looking like we're going to need to fix (or hack around) the logic used to determine those flags...

@borsboom
Copy link
Contributor

borsboom commented Nov 1, 2017

I agree the current approach is flawed. Really, GHC's configure script ought to figure out the right flags for the system, and it looks like recent versions of GHC may do this (this comment kind of implies it).

Can anyone confirm whether recent GHC bindists work "out of the box" on these platforms (without passing any of the PIE-related flags in the configure environment variables or patching the settings file)?

If this is fixed upstream, perhaps we should just say that older GHC versions are unsupported on these platforms (but include some hints for manual patching of the settings file in the documentation).

@dkasak
Copy link
Contributor

dkasak commented Nov 1, 2017

@borsboom, not sure how recent you meant or whether this bug has the same root cause, but see #3537 for a failure of building the network package on Arch Linux until ghc-build: nopie is added to ~/.stack/config.yaml on ghc 8.0.2.

@rdnetto
Copy link
Contributor

rdnetto commented Nov 12, 2017

I think I have a rough idea of how to fix this - use the YAML file to determine only the GHC artifact (which depends on machine arch + linked libs), and set the GHC flags based on the local environment (incl. the GHC version). This means we'll need to start ignoring configure_env, but we can add a new property to override the inferred values.

@borsboom Is there a build server somewhere which builds the GHC artifacts available here? I'd like to take a look at the configure logs to check if there are any other implicit assumptions we might be missing.

@borsboom
Copy link
Contributor

borsboom commented Nov 25, 2017

Is there a build server somewhere which builds the GHC artifacts available here? I'd like to take a look at the configure logs to check if there are any other implicit assumptions we might be missing.

@rdnetto There is not. Most of these come from the GHC release team using whatever process they have. The tinfo6/ncurses6 bindists are built by us using the instructions here: https://docs.haskellstack.org/en/stable/MAINTAINER_GUIDE/#adding-a-new-ghc-version

@borsboom
Copy link
Contributor

I think I have a rough idea of how to fix this - use the YAML file to determine only the GHC artifact (which depends on machine arch + linked libs), and set the GHC flags based on the local environment (incl. the GHC version). This means we'll need to start ignoring configure_env, but we can add a new property to override the inferred values.

I think you're right, but it would be nice to future-proof this as much as possible (e.g. if the arguments change for a future GHC version, not having to release a new Stack version to support it). Maybe the Stack code can detect various cases and make a list of "flags" for those (pie, etc.), but then have a new section in the setup-info that is used to look up the actual arguments matching those flags and the GHC version. E.g. something like this:

ghc-config:  
  - flags:
      - pie
    ghc-version: '< 8.2'
    configure-env:
      CONF_CC_OPTS_STAGE2: -no-pie
      CONF_GCC_LINKER_OPTS_STAGE2: -no-pie
      CONF_LD_LINKER_OPTS_STAGE2: -no-pie
    configure-args: []

(note: example is probably incorrect, just to give an idea of the syntax)

So the idea is it would go through each item in the ghc-config list and try to match the flags and ghc-version (maybe should also match architecture and os), and use the configure environment and arguments of the first one that matches (or maybe combine from any that match? not sure, haven't throught that through).

That said, the above is overkill for just the pie/nopie case. Could also do something much simpler and have an optional nopie-configure-env for each GHC bindist, which contains the variables to set in the nopie case.

I lean toward the simpler approach, but I'd be interested in any other ideas.

@borsboom
Copy link
Contributor

In #3518 (comment), I tested myself and the tinfo-nopie variant that Stack chose seemed to basically work on fresh Arch except when building packages like network and bindings-uname which need the additional -no-pie argument passed to GCC when linking. At this point I don't think we should change the order of choosing ncurses6 vs. tinfo6 since it breaks other distros, and instead figure out exactly when this extra -no-pie argument needs to be set.

@borsboom
Copy link
Contributor

Once the work on solving #3518 is done it should no longer matter whether tinfo6 or ncurses6 is preferred to have a working GHC (using that as a "proxy" to decide on unrelated things was wrong), so I'm going to close this.

All the latest versions of distros I looked at that upgraded from ncurses5 have both tinfo6 and ncurses6 now, so we may just be able to pick one of them instead of supporting both. Better, Stack could check for any option that could work and then use whichever has a bindist available.

@borsboom borsboom closed this Dec 24, 2017
borsboom referenced this pull request Dec 27, 2017
These changes are motivated by #3636.

* `stack setup` looks for GHC bindists and installations by any OS key
  that is compatible (rather than only checking a single one).   This is
  relevant on Linux where different distributions may have different
  combinations of libtinfo 5/6, ncurses 5/6, and gmp 4/5, and will allow
  simpifying the setup-info metadata YAML for future GHC releases.

* `stack setup` no longer uses different GHC configure options on Linux
  distributions that use GCC with PIE enabled by default.  GHC detects
  this itself since ghc-8.0.2, and Stack's attempted workaround for older
  versions caused more problems than it solved.
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.

7 participants