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

Infer target platform from ghc --info #1214

Merged
merged 3 commits into from Mar 1, 2013

Conversation

Projects
None yet
7 participants
@lukexi
Copy link
Collaborator

lukexi commented Feb 20, 2013

Add a new operating system: iOS

Base install paths on target platform

Thread platform through Cabal

Infer target platform from ghc --info

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Feb 20, 2013

I don't think that this is what Johan meant :-)

You have one large commit that contains all your changes instead of a series of relatively small and self-contained patches where each one can be reviewed separately.

Also, you don't have to close the pull request if you just want to edit history with rebase: you can just do push -f to the relevant branch and the pull request will be automatically updated.

@tibbe

This comment has been minimized.

Copy link
Member

tibbe commented Feb 21, 2013

What @23Skidoo said. It looks like this could e.g. be 4 commits, based on the four sentences in the commit message. Thanks for your patience!

@lukexi

This comment has been minimized.

Copy link
Collaborator

lukexi commented Feb 21, 2013

Sorry about that : ) — here it is again split into 4 logical commits.

b3594cb, 8eaf9a1 and fab3dd7 should work in isolation and b1c6200 depends on 8eaf9a1.

@tibbe

This comment has been minimized.

Copy link
Member

tibbe commented Feb 22, 2013

Could someone clarify what the current buildPlatform variable means? Is it the platform GHC was built on (as described in http://hackage.haskell.org/trac/ghc/wiki/CrossCompilation)?

@dcoutts

This comment has been minimized.

Copy link
Member

dcoutts commented Feb 23, 2013

I think putting the Platform into the Compiler type is not right, it should go into the LocalBuildInfo directly. It's not just the compiler that varies between platforms, it's the whole toolchain.

Fixing this should not be hard. In the configCompiler, just return (Compiler, Maybe Platform, ProgramConfiguration), ie return the platform as a separate result. (Of course all other compilers other than GHC will return Nothing.) Then if it's not Nothing, put it into the LocalBuildInfo, or default to the build platform. This also leaves open the option later to take the target platform as a command line arg, rather than inferring based on the ghc.

Longer term, I expect we'll need more info for the target, a whole target profile with info about the whole toolchain, default c flags etc etc. And the appropriate place for that is in the LocalBuildInfo, so that's the other reson we should make sure we start by putting the Platform id in the right place.

@lukexi

This comment has been minimized.

Copy link
Collaborator

lukexi commented Feb 24, 2013

Hi Duncan, that all sounds great. I'll make the changes later today. Thanks for reviewing!

@lukexi

This comment has been minimized.

Copy link
Collaborator

lukexi commented Feb 27, 2013

Here are those changes — let me know if that looks good and I'll tidy up the history.

@dcoutts

This comment has been minimized.

Copy link
Member

dcoutts commented Feb 28, 2013

That looks good, thanks. Can I make one nitpick?

So we have the "build" platform and the "host" platform. (I had to go look it up to make sure I've got the right terminology!) (Note also, that there's no such thing as the "target" platform in our context, that only makes sense for compilers.)

In Distribution.System we have the right name buildPlatform :: Platform. So to keep things clear, we should also use the terminology "host" in the code. So we ought really to call it hostPlatform :: Platform, ie the field in LocalBuildInfo should be hostPlatform rather than just platform.

Sorry about asking you to go do more renaming, but it's probably best to establish that naming convention from the start rather than people gettting confused later about build vs host.

We should then (I don't mean you have to do this in this patch!) audit our other uses of buildPlatform and see which ought to be changed to hostPlatform. I suspect there's several, in particular in the cabal config flags, if you use "if arch(blah)", that probably ought to refer to the host not the build platform (and we may decide we need both). Anyway, that's for later.

@lukexi

This comment has been minimized.

Copy link
Collaborator

lukexi commented Feb 28, 2013

No problem! It's tricky to keep straight so I'm all for extra clarity : ). I'll do that now.

@lukexi

This comment has been minimized.

Copy link
Collaborator

lukexi commented Feb 28, 2013

^ Tidied up.

return (comp, compPlatform, conf4)

targetPlatform :: [(String, String)] -> Platform
targetPlatform ghcInfo = fromMaybe buildPlatform maybePlatform

This comment has been minimized.

@tibbe

tibbe Mar 1, 2013

Member

You already do this defaulting in Configure. Couldn't you just return Nothing here on failure instead of duplicating the defaulting?

@tibbe

This comment has been minimized.

Copy link
Member

tibbe commented Mar 1, 2013

This looks great. Just added one minor comment and then we're good to merge.

lukexi added some commits Feb 28, 2013

Return Maybe Platform as part of compiler configure, and place it in …
…LocalBuildInfo as hostPlatform.

GHC infers the platform form ghc --info using new 'platformFromTriple' function. Other compilers return Nothing, which triggers fallback to old behavior of using buildPlatform. hostPlatform is then threaded through to initialPathTemplateEnv.
@lukexi

This comment has been minimized.

Copy link
Collaborator

lukexi commented Mar 1, 2013

Nice catch, thanks!

@tibbe tibbe merged commit fa44826 into haskell:master Mar 1, 2013

@tibbe

This comment has been minimized.

Copy link
Member

tibbe commented Mar 1, 2013

All merged. Thanks for the patches!

-> PackageDescription
-> IO a -> IO a
withWin32SelfUpgrade _ _ _ _ action | buildOS /= Windows = action
withWin32SelfUpgrade verbosity configFlags compid pkg action = do
withWin32SelfUpgrade _ _ _ _ _ action | buildOS /= Windows = action

This comment has been minimized.

@23Skidoo

23Skidoo Mar 3, 2013

Member

Shouldn't you replace the buildOS /= Windows check with platformOS platform /= Windows?

@nh2

This comment has been minimized.

Copy link
Member

nh2 commented Jul 31, 2013

Could we change this such that public API like configCompiler still returns the old tuple?

This change breaks almost all packages that use a custom Setup.hs, and the only way for them to support multiple cabal versions would be CPP.

Maybe we could add another function that returns the triple, and deprecate the old one.

@cartazio

This comment has been minimized.

Copy link
Member

cartazio commented Aug 15, 2013

you can't use CPP that branches on lib versions in setup.hs, so you'd have to use template haskell with hand written ASTs

@cartazio

This comment has been minimized.

Copy link
Member

cartazio commented Aug 15, 2013

breaking changes to the cabal apis need to probably go through a release or so of just being deprecated before being removed, else its really really hard for maintainers to support multiple cabal versions. (though gnarly setup.hs files are breakage prone by their very nature)

@dagit

This comment has been minimized.

Copy link
Collaborator

dagit commented Aug 15, 2013

@cartazio and the additional limitation that you can't specify what version of Cabal a Setup.hs file depends (otherwise, that might provide an escape hatch for some API changes).

@cartazio

This comment has been minimized.

Copy link
Member

cartazio commented Aug 15, 2013

@dagit there is a hacky work around, but its basically doing poor man's CPP

used it to patch the bos/llvm lib to work with cabal 1.17

{-
this is the workaround for conditional compilation if template haskell was more
    permissive, but isn't

 --- what i'd like to write, but can't because template haskell rejecting
 the branch that has the wrong api version
 extractCLBI x=
     $(if cabalVersion >= Version [1,17,0] []
         then [|  getComponentLocalBuildInfo 'x CLibName  |]
         else  [|
                    let   LocalBuildInfo  { libraryConfig = Just clbi } = 'x
                        in clbi |]
    )


-}

--- horrible hack to support cabal versions both above and below 1.17
extractCLBI x=
    $(if cabalVersion >= Version [1,17,0] []
        then  appE (appE  ( varE $ mkName "getComponentLocalBuildInfo") ( varE 'x) ) (conE ( mkName "CLibName"))

        else  letE
                [valD  (recP
                            (mkName "LocalBuildInfo" )
                            [fieldPat (mkName "libraryConfig")
                             (conP (mkName "Just")    [varP $ mkName "clbi"] ) ] )
                    (normalB $ varE 'x)   []    ]
                 (varE $ mkName "clbi")  )

[edit: fixed quoting]

@nh2

This comment has been minimized.

Copy link
Member

nh2 commented Aug 15, 2013

@cartazio

you can't use CPP that branches on lib versions in setup.hs

If I understand it right, you agree with me that this API change is problematic?

@cartazio

This comment has been minimized.

Copy link
Member

cartazio commented Aug 15, 2013

i'm saying that its currently a pain in the ass (albeit doable hackily) to work around these api changes, but it really sucks, and will only get fixed by proactive maintainers.

the real problem is theres no way to have setup.hs have CPP than usages the cabal library version

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Aug 15, 2013

@cartazio

i can't quote this block properly, but you get the idea

Use ````` (three backticks) instead of '''.

@cartazio

This comment has been minimized.

Copy link
Member

cartazio commented Aug 16, 2013

@23Skidoo thanks, Fixed

23Skidoo added a commit to 23Skidoo/cabal that referenced this pull request Aug 23, 2013

Backwards compatibility patch for cross-compilation changes.
Changes back the types of 'configCompiler' and 'configCompilerEx', but marks
them as deprecated. Adds new functions 'configCompilerEx' and
'configCompilerAuxEx'.

The remaining functions that changed type after the cross-compilation changes
shouldn't matter from the backwards compatibility standpoint:

    * InstallDirs.{absoluteInstallDirs, prefixRelativeInstallDirs,
      initialPathTemplateEnv} - the versions in D.S.LocalBuildInfo retain their
      old types and are usually used instead. In any case, removing the Platform
      parameter from here is problematic because the default install dirs now
      include $arch and $os vars.

    * D.S.Configure.configure - shouldn't be used by the setup scripts.

See haskell#1214 for the original (backwards-incompatible) patches.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment