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

basedir #4874

Merged
merged 6 commits into from
Feb 14, 2018
Merged

basedir #4874

merged 6 commits into from
Feb 14, 2018

Conversation

angerman
Copy link
Collaborator

@angerman angerman commented Nov 8, 2017

Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!

@angerman angerman changed the title Feature/basedir Feature/basedir [WIP] Nov 8, 2017
@@ -332,6 +332,12 @@ data ConfigFlags = ConfigFlags {

configDistPref :: Flag FilePath, -- ^"dist" prefix
configCabalFilePath :: Flag FilePath, -- ^ Cabal file to use
configBaseDir :: Flag FilePath, -- ^ The directory containing the
Copy link
Member

Choose a reason for hiding this comment

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

So now takeBaseName configCabalFilePath == configBaseDir doesn't need to hold? I still don't quite understand why you need a new option and can't use takeBaseName $ configCabalFilePath.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are not bound to have the configCabalFilePath. E.g. given the following invocation:

defaultMainWithHooksNoReadArgs hooks gpd ["--basedir", "/path/to/happiness"]

you do not have the --cabalConfigFilePath.

And I'm not sure we want to force providing the --cabalConfigFilePath? If we can agree that we want to require the --cabalConfigFilePath in those cases. I'm happy to change to takeBaseName.

Copy link
Member

Choose a reason for hiding this comment

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

You are not bound to have the configCabalFilePath.

But this isn't different from the situation when configBaseDir is NoFlag. Your example can be written as defaultMainWithHooksNoReadArgs hooks gpd ["--cabal-file", "/path/to/happiness/happiness.cabal"] with --cabal-file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right. the more verbose solution, might be the less error prone. Let's do it that way.

@mgsloan mgsloan mentioned this pull request Nov 8, 2017
angerman added a commit to angerman/hadrian that referenced this pull request Nov 9, 2017
@angerman angerman force-pushed the feature/basedir branch 2 times, most recently from b37a97d to e0b806f Compare November 10, 2017 14:19
(programInvocation (sh {programOverrideEnv = overEnv}) args')
{ progInvokeCwd = Just (baseDir lbi) }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe this change breaks on windows. I'm not sure exactly why yet.

@angerman angerman force-pushed the feature/basedir branch 4 times, most recently from 25e6776 to b8fe569 Compare November 12, 2017 09:04
@angerman
Copy link
Collaborator Author

@hvr, @23Skidoo why did Jenkins not test this one? How can we force a check? Can we have some post to the PR trigger (e.g. [rerun Travis]) trigger a rerun of the CI?

@23Skidoo
Copy link
Member

@angerman idk, it does this sometimes. Maybe connectivity issues between GH and Travis. Try making a small tweak (so that commit hash gets changed) and force-pushing.

@23Skidoo
Copy link
Member

23Skidoo commented Nov 12, 2017

@angerman Also, if you have permissions, you can go to Settings -> Webhooks and click "Re-deliver" on the offending ones. I see quite a lot of failed Travis calls there. Oh, wait, Travis integration settings are actually in "Integrations & services", and failed webhook calls were for travis-dedup, which we for some reason still had enabled. Removed that one. Haven't found a way to manually re-deliver Travis calls.

@angerman angerman closed this Nov 13, 2017
@angerman angerman reopened this Nov 13, 2017
@angerman
Copy link
Collaborator Author

Let's see if this does the trick.

@angerman
Copy link
Collaborator Author

@23Skidoo, @hvr, while this works for what I'm doing to the GHC build system. I believe there are still some code path's left. Any creative idea how to test this properly and get the sufficient coverage of the cabal codebase?

@angerman
Copy link
Collaborator Author

Note to self: need to figure out why test fail.

@angerman angerman changed the title Feature/basedir [WIP] basedir Jan 18, 2018
@angerman angerman self-assigned this Jan 19, 2018
@angerman
Copy link
Collaborator Author

@23Skidoo, @ezyang this also fails with downstream travis.

I'd really like to get this merged. As it's a prerequisite for the relocatable hadrian (building ghc with shake) snowleopard/hadrian#445 PR, and I'm trying to tie up loose ends here.

@angerman
Copy link
Collaborator Author

angerman commented Feb 8, 2018

I believe something on master started breaking windows :(

@angerman
Copy link
Collaborator Author

angerman commented Feb 9, 2018

Rebased onto #5132

# Conflicts:
#	Cabal/Distribution/Simple.hs
If we have a cabalFilePath, just invoke the configure script there.  Otherwise
try to invoke it locally to the CWD.  But don't try to shell out in a different
directory, that would mess up the paths.  In general we want to run
/path/to/configure from the bulid directory (e.g. outside of the package folder).
@angerman
Copy link
Collaborator Author

rebased onto master

@angerman angerman added this to the 2.2 milestone Feb 10, 2018
@angerman
Copy link
Collaborator Author

all green!

@angerman
Copy link
Collaborator Author

@23Skidoo can we get this merged now?

@23Skidoo
Copy link
Member

I'll merge this later today or tomorrow, there are some small changes I want to make. Sorry for the delay, I've been ill.

@angerman
Copy link
Collaborator Author

@23Skidoo thanks!

Copy link
Member

@23Skidoo 23Skidoo left a comment

Choose a reason for hiding this comment

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

I have some comments, but basically this is IMO mergeable.

srcHeaders <- forM relIncDirs $ \dir ->
fmap (dir </>) . filter isHeader <$> listDirectory (baseDir lbi </> dir)
`catchIO` (\_ -> return [])
let commonHeaders = concat genHeaders `intersect` concat srcHeaders
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't catch stuff like #include "foo/foo.h" or #include "../../foo.h". The latter we can ignore I think, for the former we'll need to walk the directory recursively.

@@ -487,7 +493,13 @@ sanityCheckHookedBuildInfo pkg_descr (_, hookExes)

sanityCheckHookedBuildInfo _ _ = return ()

-- | Try to read the 'localBuildInfoFile'
tryGetBuildConfig :: UserHooks -> Verbosity -> FilePath
Copy link
Member

Choose a reason for hiding this comment

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

Weird, I was quite sure we already had this function somewhere...

getHookedBuildInfo verbosity
readHookWithArgs get_verbosity get_dist_pref _ flags = do
dist_dir <- findDistPrefOrDefault (get_dist_pref flags)
getHookedBuildInfo (dist_dir </> "build") verbosity
Copy link
Member

Choose a reason for hiding this comment

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

I'm not totally keen on the fact that we're hardcoding "build" everywhere. This works with new-build directory structure, right?

getHookedBuildInfo :: Verbosity -> IO HookedBuildInfo
getHookedBuildInfo verbosity = do
maybe_infoFile <- defaultHookedPackageDesc
getHookedBuildInfo :: FilePath -> Verbosity -> IO HookedBuildInfo
Copy link
Member

Choose a reason for hiding this comment

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

We should mention in the changelog/migration guide that this function has changed type.

@@ -1536,6 +1536,7 @@ findPackageDesc dir
tryFindPackageDesc :: FilePath -> IO FilePath
tryFindPackageDesc dir = either die return =<< findPackageDesc dir

{-# DEPRECATED defaultHookedPackageDesc "Use findHookedPackageDesc with the proper base directory instead" #-}
-- |Optional auxiliary package information file (/pkgname/@.buildinfo@)
Copy link
Member

Choose a reason for hiding this comment

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

This should also be mentioned in the changelog.

@23Skidoo
Copy link
Member

Just waiting for it to go green now...

@23Skidoo 23Skidoo merged commit 783cbe6 into haskell:master Feb 14, 2018
@23Skidoo
Copy link
Member

Merged, thanks!

++ PD.includeDirs bi
-- potential includes generated by `configure'
-- in the build directory
++ [buildDir lbi </> dir | dir <- PD.includeDirs bi],
Copy link
Member

Choose a reason for hiding this comment

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

Don't you need to filter out absolute paths here as well?

23Skidoo added a commit that referenced this pull request Mar 2, 2018
This reverts commit 783cbe6, reversing
changes made to c094940.
@ezyang
Copy link
Contributor

ezyang commented Apr 1, 2018

Mac OS X regression caused by this: https://ghc.haskell.org/trac/ghc/ticket/14972

In general, the new files in build can cause GHCi to stop working if one of the files/directory unluckily has the same name as a dynamic library (gmp in this case), because then gcc -print-file-name goes insane (but apparently only on OS X)

@angerman
Copy link
Collaborator Author

angerman commented Apr 1, 2018

I've created https://phabricator.haskell.org/D4553, which should ensure that what ever gcc --print-file-name returns is verified to be a file prior to passing it on.

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.

4 participants