Skip to content

Put command line --with-PROG into AllPackages #5018

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

Merged
merged 3 commits into from
Feb 8, 2018

Conversation

angerman
Copy link
Collaborator

@angerman angerman commented Jan 12, 2018

This should help with #4939. --with-PROG flags should be applied to every
package. If we change hsc2hs via --with-hsc2hs= or any other program and
similarly --hsc2hs-options should also be provided to that program. Same
holds for --PROG-options. Because we do no inherit the properties from
AllPackages for LocalPackages, we need to have the set of programs and options
in both.

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
Copy link
Collaborator Author

What's up with travis? Jobs take for ever and die with random (e.g. apt-get) errors...

@ezyang
Copy link
Contributor

ezyang commented Jan 16, 2018

I dunno what's up with Travis, but if it's a build system failure keep refreshing the failing build until it passes.

I know this is orthogonal to the problem you are trying to solve, but could you take a look over PackageConfig and see if there are other fields we want to apply globally? Is there a simple rule which we can apply to make this decision one way or another?

@angerman
Copy link
Collaborator Author

@ezyang yea, I keep restarting them. Let's see if they eventually turn green.

-- | Project configuration that is specific to each package, that is where we
-- can in principle have different values for different packages in the same
-- project.
--
data PackageConfig
   = PackageConfig {
       -- The following should be applied to _all_ package, I believe.
       packageConfigProgramPaths        :: MapLast String FilePath,
       packageConfigProgramArgs         :: MapMappend String [String],
       packageConfigProgramPathExtra    :: NubList FilePath,

       -- these are specific to single packages
       packageConfigFlagAssignment      :: FlagAssignment,
       packageConfigVanillaLib          :: Flag Bool,
       packageConfigSharedLib           :: Flag Bool,
       packageConfigStaticLib           :: Flag Bool,
       packageConfigDynExe              :: Flag Bool,

       -- not sure abount profiling.
       packageConfigProf                :: Flag Bool, --TODO: [code cleanup] sort out
       packageConfigProfLib             :: Flag Bool, --      this duplication
       packageConfigProfExe             :: Flag Bool, --      and consistency
       packageConfigProfDetail          :: Flag ProfDetailLevel,
       packageConfigProfLibDetail       :: Flag ProfDetailLevel,

       -- XXX: this is a tricky one. We would want things like --host= to be GLOBAL
       --          while other cofngiure flags we might want local only.
       packageConfigConfigureArgs       :: [String],

       -- tricky, but covered by all-packages.
       packageConfigOptimization        :: Flag OptimisationLevel,

       -- tricky, do we want $host-<name>, I somewhat doubt it.
       -- as such it's local, or all-packages.
       packageConfigProgPrefix          :: Flag PathTemplate,
       packageConfigProgSuffix          :: Flag PathTemplate,

       -- Again, not sure we'd want to force these to be global,
       -- I'd argue that all-packages is the rigth place for these.
       packageConfigExtraLibDirs        :: [FilePath],
       packageConfigExtraFrameworkDirs  :: [FilePath],
       packageConfigExtraIncludeDirs    :: [FilePath],
       packageConfigGHCiLib             :: Flag Bool,
       packageConfigSplitSections       :: Flag Bool,
       packageConfigSplitObjs           :: Flag Bool,
       packageConfigStripExes           :: Flag Bool,
       packageConfigStripLibs           :: Flag Bool,
       packageConfigTests               :: Flag Bool,
       packageConfigBenchmarks          :: Flag Bool,
       packageConfigCoverage            :: Flag Bool,
       packageConfigRelocatable         :: Flag Bool,
       packageConfigDebugInfo           :: Flag DebugInfoLevel,
       
       -- No idea about these. 
       packageConfigRunTests            :: Flag Bool, --TODO: [required eventually] use this
       packageConfigDocumentation       :: Flag Bool, --TODO: [required eventually] use this
       packageConfigHaddockHoogle       :: Flag Bool, --TODO: [required eventually] use this
       packageConfigHaddockHtml         :: Flag Bool, --TODO: [required eventually] use this
       packageConfigHaddockHtmlLocation :: Flag String, --TODO: [required eventually] use this
       packageConfigHaddockForeignLibs  :: Flag Bool, --TODO: [required eventually] use this
       packageConfigHaddockExecutables  :: Flag Bool, --TODO: [required eventually] use this
       packageConfigHaddockTestSuites   :: Flag Bool, --TODO: [required eventually] use this
       packageConfigHaddockBenchmarks   :: Flag Bool, --TODO: [required eventually] use this
       packageConfigHaddockInternal     :: Flag Bool, --TODO: [required eventually] use this
       packageConfigHaddockCss          :: Flag FilePath, --TODO: [required eventually] use this
       packageConfigHaddockHscolour     :: Flag Bool, --TODO: [required eventually] use this
       packageConfigHaddockHscolourCss  :: Flag FilePath, --TODO: [required eventually] use this
       packageConfigHaddockContents     :: Flag PathTemplate, --TODO: [required eventually] use this
       packageConfigHaddockForHackage   :: Flag HaddockTarget
     }
deriving (Eq, Show, Generic)

We have all-packages (every time I type that, I wonder if we should rather go with package * for consistency; all-packages is yet another key word I need to remember while package * feels much more natural to me...) which gives us a nice middle ground and end user control(!) about what is being passed to all packages or just to specific ones.

For all the fields, it's only the configure options for which we likely would want the --host only part to be global. That does feel like a really fiddly thing to get right (magically filtering [String] and not screwing up).

As a general rule:

  • Anything that's host/toolchain/env specific (programs mostly) should be global, you can't really mix
    different host/toolchain/environments (Build/Link two different architectures in one go and
    expect the result to still expect this to work).
  • Anything else should be user controllable (what flags, configurations, ... to apply to packages)

This PR addresses the issue at the command line parsing point. As such I'm a bit conflicted about the ConfigureArgs.

@ezyang
Copy link
Contributor

ezyang commented Jan 18, 2018

every time I type that, I wonder if we should rather go with package * for consistency; all-packages is yet another key word I need to remember while package * feels much more natural to me...

I don't have any problem with package * :)

Your decision criteria seems reasonable to me, let's put it in the code!

@23Skidoo
Copy link
Member

23Skidoo commented Jan 18, 2018

package * was also suggested by @hvr during the discussion on the original all-packages PR, and everyone seemed to agree that it's a good idea. Patches welcome.

@angerman
Copy link
Collaborator Author

@ezyang, @23Skidoo see #5053 for package *.

@angerman
Copy link
Collaborator Author

I hereby declare Travis dead.

@angerman
Copy link
Collaborator Author

@ezyang the following should be resolved now:

Your decision criteria seems reasonable to me, let's put it in the code!

@angerman angerman mentioned this pull request Jan 18, 2018
2 tasks
@ezyang
Copy link
Contributor

ezyang commented Jan 18, 2018

Alright, LGTM.

@angerman
Copy link
Collaborator Author

angerman commented Jan 19, 2018

Looks like Travis macOS builder are hopelessly overlaoded.

Also there is one run in which travis fails...

$ ./Cabal/misc/travis-diff-files.sh
+git status
+git diff-files -p --exit-code
diff --git a/cabal-install/cabal-install.cabal b/cabal-install/cabal-install.cabal
index 5cd1014..18a9f9d 100644
--- a/cabal-install/cabal-install.cabal
+++ b/cabal-install/cabal-install.cabal
@@ -65,6 +65,7 @@ Extra-Source-Files:
   tests/IntegrationTests2/targets/exes-disabled/cabal.project
   tests/IntegrationTests2/targets/exes-disabled/p/p.cabal
   tests/IntegrationTests2/targets/exes-disabled/q/q.cabal
+  tests/IntegrationTests2/targets/lib-only/p.cabal
   tests/IntegrationTests2/targets/libs-disabled/cabal.project
   tests/IntegrationTests2/targets/libs-disabled/p/p.cabal
   tests/IntegrationTests2/targets/libs-disabled/q/q.cabal

maybe this is due to travis merge logic?

This should help with haskell#4939. `--with-PROG` flags should be applied to every
package.  If we change hsc2hs via `--with-hsc2hs=` or any other program and
similarly `--hsc2hs-options` should also be provided to that program.  Same
holds for `--PROG-options`.  Because we do no inherit the properties from
AllPackages for LocalPackages, we need to have the set of programs and options
in both.
@ezyang
Copy link
Contributor

ezyang commented Jan 19, 2018

That error says that you forgot to regenerated some autogenerated parts of cabal-install.cabal. Run update-cabal-files.sh

@phadej
Copy link
Collaborator

phadej commented Jan 19, 2018

@ezyang @angerman rather run make gen-extra-source-files

@phadej
Copy link
Collaborator

phadej commented Jan 19, 2018

... though I already pushed the fix to master, so no need. rebase would be enough if you really want to rerun all the Travis.

@angerman
Copy link
Collaborator Author

If we agree this is good to go, I’m fine with merging without stressig poor Travis once again. macOS builds seem dead for the time being anyway, due to some capacity issues at travisci.

@angerman
Copy link
Collaborator Author

angerman commented Feb 4, 2018

Whee! It's finally green, can we merge it?

@angerman
Copy link
Collaborator Author

angerman commented Feb 8, 2018

@23Skidoo can we merge this?

@23Skidoo 23Skidoo merged commit ef72094 into haskell:master Feb 8, 2018
@23Skidoo
Copy link
Member

23Skidoo commented Feb 8, 2018

Merged, 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.

LGTM.

@dalaing
Copy link
Collaborator

dalaing commented Mar 6, 2018

Gah, misclicked while looking for the names of milestones, ignore me.

angerman added a commit to zw3rk/cabal that referenced this pull request Mar 8, 2018
In haskell#5018, I noted, that `packageConfigConfigureArgs` is a tricky field.

I have since then come to the conclusion that most of the time you actually
do want the `--configure-options` to be passed down.
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.

5 participants