Propagate section-specific data in conditionals. #175

scott-fleischman · 2017-05-18T23:16:29Z

This is a working approach to allowing section-specific properties in conditionals.

The main goal is to allow library-specific properties in the body of conditionals. For example:

library:
  source-dirs: src
  exposed-modules: Foo
  other-modules: []
  when:
    condition: os(windows)
    exposed-modules:
    - Bar

Implementation details:

Effectively I propagated the type parameter for a Section throughout all of the conditional-related code.

This changes the existing behavior in a few instances.

You can now specific properties like github within a conditional for the package (top-level properties). One previous test assumed that was invalid, and I changed the test to allow that case.
Package-specific properties within conditionals allow underscore prefixes, because all package-specific do. It is less work to leave it like this. Note that it does require a change to a test which assumed that wasn't allowed before.
executableSectionMain in ExecutableSection now needs to be Maybe FilePath instead of FilePath. This is so that you don't have to specify main in every conditional in an executable section (which would break existing code). Note that this pushes the validation that main is supplied to a different area in the codebase. The new validation code works but could be improved.

I added a FlatThen as an analogous type to ThenElse which handles parsing the flat condition property with its sibling section properties.

Note of caution: it is recommended to always set other-modules (and exposed-modules in the library section) at the root level when using one or both properties within a conditional. Else the recursive file selection behavior will likely conflict with what the conditional is trying to achieve. Note that in the code, the conditionals do not expand empty exposed-modules and other-modules. That is only done at the first level of the library and executable sections.

scott-fleischman · 2017-05-18T23:16:53Z

Fixes #116

scott-fleischman · 2017-05-19T16:56:37Z

I force-pushed an update to my branch that fixes a couple issues with rendering. I have tried it on our real-world use cases, and they work as expected.

scott-fleischman · 2017-05-19T17:09:18Z

We might consider offering a warning when exposed-modules and/or other-modules is used within a conditional and one of them has the default value at the root level (meaning it will fill its value recursively).

For example:

library:
  source-dirs: src
  exposed-modules: Foo
  when:
    condition: os(windows)
    exposed-modules: Bar

will generate:

library:
  exposed-modules: Foo
  other-modules:
  - Bar
  - Paths_foo
  if os(windows)
    exposed-modules: Bar

Note that it includes Bar in other-modules outside of the conditional, which is unlikely the desired result.

sol · 2017-05-21T12:02:39Z

@scott-fleischman Hey, on a first glance this looks reasonable. I will do a proper review when I have time, but it may take a while.

sol · 2017-06-06T14:25:04Z

src/Hpack/Config.hs


-instance FromJSON ThenElse where
+instance (HasFieldNames a, FromJSON a) => FromJSON (ThenElse a) where
  parseJSON = genericParseJSON_

 data Empty = Empty


Can Empty be removed now?

Empty is not needed in the library code. It is still used in tests, mainly for the ToJSON and HasFieldNames instances. I'll move Empty to the test Helper module.

Cool, makes sense.

Late here already, I'll try to continue the review tomorrow (no promise).

sol · 2017-06-07T00:26:05Z

@scott-fleischman If I specify e.g. github in a global conditional then it has no effect, it's just ignored, right?

I'm puzzled whether we want to parametrize Section twice, once for it's additional top-level fields and once for the fields allowed in a conditional. We would then use:

Section LibrarySection LibrarySection for libraries
Section ExecutableSection ExecutableSection for executables, tests and benchmarks
Section PackageConfig Empty for the package itself

Any thoughts?

scott-fleischman · 2017-06-07T15:47:30Z

I'll give that approach a try and see how it goes.

scott-fleischman · 2017-06-16T00:33:27Z

@sol I force-pushed an update that uses Section c d where c is the Section data for the conditionals and d is the Section data for the root section. The d so that Section c can be a Functor over the sectionData field.

The code builds, but the tests aren't passing yet. There are a few bugs in the unknown fields code still. But mainly I wanted to get your feedback whether this is the direction you would like this implementation to go. The Section code gets messier because of the two type arguments. And I had to define the Functor instance for Conditional manually.

I kind of prefer the older way due to its simplicity. This way makes the Section tree rather subtle in how the conditional type argument gets propagated in the children.

scott-fleischman · 2017-06-16T00:34:36Z

I will also have to rebase my commit(s) in order to fix the conflicts.

Just let me know whether you want Section c d or whether to revert back to Section a.

Thanks!

sol · 2017-06-24T01:11:42Z

@scott-fleischman Sorry for the late response again. I think we should reject e.g. github in top-level conditionals, as that is incorrect. Unless we find a different way to achieve this, I would go with this approach. But then again, I haven't written the code and may have the wrong feeling on how messy this approach gets. Just from a quick glance it looks better than I had imagined.

If you are ok with this approach, please go ahead and rebase.

scott-fleischman · 2017-09-11T15:58:40Z

@sol I finally got the motivation to finish this PR, rebasing off the latest master and fixing the broken tests. Let me know what you think!

scott-fleischman · 2017-09-11T16:00:02Z

I basically just started from the master branch and re-applied the same work piece by piece in order to isolate each step (mainly so I could find the source of the previous broken tests).

scott-fleischman · 2017-09-11T16:04:53Z

Personally having two type parameters for Section is a bit verbose, but mainly confusing. Section b a uses b for the conditional branch and a for the data field. It's easy to get those backwards, which is what was the source of the earlier test failures.

One consideration is whether we want to add a type synonym for Section a a (perhaps type Section' a = Section a a), since Section ExecutableSection ExecutableSection gets a bit verbose. However, I tend to avoid type synonyms so I didn't include them initially.

scott-fleischman · 2017-09-14T17:44:25Z

I tested the code against our real-world use case and it works great.

scott-fleischman · 2017-09-14T18:01:47Z

In the cabal project, you can see a few uses of these features.

In the Cabal.cabal here and here

In tests: here, here and here

scott-fleischman · 2017-09-14T18:06:14Z

@sol as you questioned the validity of fields like github in a top-level conditional, I did a cursory investigation of the cabal code and couldn't find a situation where that would be possible in a cabal file. But I did see usages of other-modules and exposed-modules as I pointed out in my previous comment.

The current code in this PR reflects both of these (omits top-level package-specific fields in conditionals, but supports library and executable-specific fields in conditionals.

scott-fleischman · 2017-10-10T20:41:07Z

@sol At this point, what are your thoughts on this PR and/or the issue it's related to?

sol · 2017-10-12T16:34:36Z

@scott-fleischman I'm very much sorry for being unresponsive on this. The reason is lack of cycles on my side and that this PR is somewhat biggish.

I'll be busy tomorrow and for the weekend, but I'll try to get back at this early next week (no promise).

sol · 2017-10-27T13:24:37Z

@scott-fleischman I ended up doing some significant refactoring first, then implementing this on top of that based on your code. I'll add some comments to this PR on what I changed.

Can you please test if what is on master works for you?

sol · 2017-10-27T14:19:52Z

src/Hpack/Config.hs

+data FlatThen a = FlatThen {
+  _flatThenCondition :: String
+, _flatThenBody :: CaptureUnknownFields (Section a a)
+} deriving (Eq, Show, Generic)


What we do here with FlatThen is essentially saying: On this level we want to accept all the fields of Condition and all the fields of Section. I added a Product data type that allows us to solve this kind of situation generically. So instead of FlatThen we can have Product (Section a a) Condition now.

sol · 2017-10-27T14:22:58Z

src/Hpack/Config.hs

 , sectionBuildTools :: Dependencies
-} deriving (Eq, Show, Functor, Foldable, Traversable)
+} deriving (Show)


I noticed that in the parsed result we actually don't need two type parameters (we only have Section Library Library and Section Executable Executable). The second type parameter is only needed during parsing.

As a second observation: We have types that represent the syntactical structure of package.yaml that we use for parsing the actual YAML. We then map those syntactic types to types that represent a Package semantically (e.g. ExecutableConfig is mapped to Executable).

For Section the syntactic type is CommonOptions, but here we break the strict separation between syntactic and semantic types. We use Section during parsing the actual YAML as well.

What I did is: Remove the FromJSON instance for Section and only use CommonOptions during parsing.

sol · 2017-10-27T14:30:33Z

src/Hpack/Config.hs

@@ -416,12 +442,12 @@ data Library = Library {
 } deriving (Eq, Show)

 data Executable = Executable {
-  executableName :: String
-, executableMain :: FilePath
+  executableName :: Maybe String


executableName can never be Nothing and is never valid inside a conditional. So what we want here is a type that represents this properly. There are probably several ways to achieve this, what I did is:

Change [Section Executable] to Map String (Section Executable) in Package and remove executableName from Executable. So what we have now is a mapping from executable names to executable sections.

sol · 2017-10-27T15:05:52Z

src/Hpack/Config.hs

+        validateExecutable :: Section Executable Executable -> Either String ()
+        validateExecutable ex = case executableMain . sectionData $ ex of
+          Nothing -> Left "Missing main"
+          Just _ -> Right ()


From what I tested, Cabal does not require main top-level, e.g. the following is a valid executable section:

executable foo if os(windows) buildable: False else hs-source-dirs: src, driver main-is: Main.hs

So just omitting validateExecutable is fine I guess.

sol · 2017-10-27T15:10:24Z

src/Hpack/Config.hs

          where
            inferModules :: IO [String]
            inferModules = filterMain . (++ [pathsModule]) . concat <$> mapM (getModules dir) sectionSourceDirs

            pathsModule = pathsModuleFromPackageName packageName_

            filterMain :: [String] -> [String]
-            filterMain = maybe id (filter . (/=)) (toModule $ splitDirectories executableSectionMain)
+            filterMain = maybe (const []) (\m -> maybe id (filter . (/=)) (toModule $ splitDirectories m)) executableSectionMain


I think const [] needs to be id instead (if there is no main we don't filter anything).

sol · 2017-10-27T15:12:15Z

src/Hpack/Config.hs


-            (mainSrcFile, ghcOptions) = parseMain executableSectionMain
+        fromExecutableSectionNamed :: ExecutableSection -> Executable
+        fromExecutableSectionNamed (ExecutableSection main other) = Executable Nothing main (fromMaybeList other)


I actually infer other-modules inside conditional too now (provided the user specified any source-dirs). Any reason we wouldn't want that?

I think that inferring other-modules by default in conditionals will make the default behavior not work very well, which unfortunately goes against one of the values of hpack--if you use the defaults things "just work".

The main issue is that the in a conditional, usually you want to exclude one or more other-modules that will break the build otherwise. That's why I think the preferred way to use conditionals is to manually specify other-modules to avoid getting inferred modules that break that build.

I will see if I can put together some examples that clarify it.

scott-fleischman · 2017-10-27T16:40:45Z

@sol thanks for looking at the PR. It's disappointing there's no record of my changes in the project, but I am glad the issue has a resolution, and with cleaner code than I had.

sol · 2017-10-27T23:30:32Z

It's disappointing there's no record of my changes in the project, but I am glad the issue has a resolution, and with cleaner code than I had.

I'm very much sorry for that. I tried to retain your authorship but with the amount of refactoring on master and the number of rebases this proved more and more laborsome so I eventually gave up on it. Not quite the same, but I gave you attribution in the CHANGELOG (d3896c1) and I'll make sure this won't happen with future contributions.

sol · 2017-10-27T23:46:10Z

I think that inferring other-modules by default in conditionals will make the default behavior not work very well, which unfortunately goes against one of the values of hpack--if you use the defaults things "just work".

The main issue is that the in a conditional, usually you want to exclude one or more other-modules that will break the build otherwise. That's why I think the preferred way to use conditionals is to manually specify other-modules to avoid getting inferred modules that break that build.

I will see if I can put together some examples that clarify it.

I couldn't come up with any example myself, where not inferring other-modules for executables would be more convenient. So yes, if you can come up with something, that would be very much appreciated!

For libraries, inferring exposed-modules / other-modules inside conditionals is less straight forward, this is why I deferred that for now.

scott-fleischman force-pushed the conditional-context branch from 241b683 to e907efd Compare May 19, 2017 16:53

scott-fleischman changed the title ~~Propagate section-specific data in conditionals. Fixes #116~~ Propagate section-specific data in conditionals. May 19, 2017

sol reviewed Jun 6, 2017

View reviewed changes

scott-fleischman force-pushed the conditional-context branch from 11c970b to e22dcdd Compare June 6, 2017 15:07

scott-fleischman force-pushed the conditional-context branch from e22dcdd to 197dc27 Compare June 16, 2017 00:28

scott-fleischman added 7 commits September 7, 2017 11:02

Add type variable to ThenElse.

f8dd79a

Add type parameter to Section for conditionals in Config.hs.

c7c1085

Finish propagating Section with two type arguments.

3975b49

Validate presence of main in executables.

21dd686

Fix incorrect propagation of field names.

bc89b1e

Add tests for conditional in ConfigSpec.

692022c

Add tests for conditional rendering with fix.

a828818

scott-fleischman force-pushed the conditional-context branch from 197dc27 to a828818 Compare September 11, 2017 15:57

Remove redundant constraint.

33e8cc5

Merge master; fix build.

c1ff35b

sol mentioned this pull request Oct 12, 2017

Internal libraries #200

Closed

sol closed this in 6a55600 Oct 27, 2017

sol reviewed Oct 27, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate section-specific data in conditionals. #175

Propagate section-specific data in conditionals. #175

scott-fleischman commented May 18, 2017 •

edited

Loading

scott-fleischman commented May 18, 2017

scott-fleischman commented May 19, 2017

scott-fleischman commented May 19, 2017

sol commented May 21, 2017

sol Jun 6, 2017

scott-fleischman Jun 6, 2017

sol Jun 6, 2017

sol commented Jun 7, 2017 •

edited

Loading

scott-fleischman commented Jun 7, 2017

scott-fleischman commented Jun 16, 2017

scott-fleischman commented Jun 16, 2017 •

edited

Loading

sol commented Jun 24, 2017

scott-fleischman commented Sep 11, 2017

scott-fleischman commented Sep 11, 2017

scott-fleischman commented Sep 11, 2017

scott-fleischman commented Sep 14, 2017

scott-fleischman commented Sep 14, 2017

scott-fleischman commented Sep 14, 2017

scott-fleischman commented Oct 10, 2017

sol commented Oct 12, 2017

sol commented Oct 27, 2017 •

edited

Loading

sol Oct 27, 2017

sol Oct 27, 2017

sol Oct 27, 2017

sol Oct 27, 2017

sol Oct 27, 2017

sol Oct 27, 2017

scott-fleischman Oct 27, 2017

scott-fleischman commented Oct 27, 2017

sol commented Oct 27, 2017

sol commented Oct 27, 2017

Propagate section-specific data in conditionals. #175

Propagate section-specific data in conditionals. #175

Conversation

scott-fleischman commented May 18, 2017 • edited Loading

scott-fleischman commented May 18, 2017

scott-fleischman commented May 19, 2017

scott-fleischman commented May 19, 2017

sol commented May 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sol commented Jun 7, 2017 • edited Loading

scott-fleischman commented Jun 7, 2017

scott-fleischman commented Jun 16, 2017

scott-fleischman commented Jun 16, 2017 • edited Loading

sol commented Jun 24, 2017

scott-fleischman commented Sep 11, 2017

scott-fleischman commented Sep 11, 2017

scott-fleischman commented Sep 11, 2017

scott-fleischman commented Sep 14, 2017

scott-fleischman commented Sep 14, 2017

scott-fleischman commented Sep 14, 2017

scott-fleischman commented Oct 10, 2017

sol commented Oct 12, 2017

sol commented Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scott-fleischman commented Oct 27, 2017

sol commented Oct 27, 2017

sol commented Oct 27, 2017

scott-fleischman commented May 18, 2017 •

edited

Loading

sol commented Jun 7, 2017 •

edited

Loading

scott-fleischman commented Jun 16, 2017 •

edited

Loading

sol commented Oct 27, 2017 •

edited

Loading