Allow specifying a stack.yaml for stack configurations #230

WorldSEnder · 2020-07-31T14:44:40Z

In response to #44 this PR adds an argument to Stack/StackMulti configurations for providing an explicit stack.yaml file to use. This is hard to do with generic additional arguments as mentioned in #44 since stackCradleDependencies has to know about it, so I decided to give it special syntax. Example hie.yaml (which I'd use for agda/agda)

cradle:
  stack:
    - path: "./src/full"
      component: Agda:lib
      stackYaml: stack-8.8.3.yaml
    - path: "./src/main"
      component: Agda:exe:agda
      stackYaml: stack-8.8.3.yaml

A few questions remain, of course, hence this is a draft.

Naming for stackYaml could simply be yaml
A small semantic change is that multistack/multicabal configurations are now allowed to only have a path key, defaulting the rest, not requiring a component. Imo, this is better behaviour anyway.
Test cases
Documentation

fendor

Hi!
Looks nice already!

I think it would be better if you could declare the stackYaml per cradle, not only per component. e.g. turn this

cradle:
  stack:
    - path: "./src/full"
      component: Agda:lib
      stackYaml: stack-8.8.3.yaml
    - path: "./src/main"
      component: Agda:exe:agda
      stackYaml: stack-8.8.3.yaml

into, e.g.

cradle:
  stack:
    - path: "./src/full"
      component: Agda:lib
    - path: "./src/main"
      component: Agda:exe:agda
stackYaml: stack-8.8.3.yaml

or something else to avoid all this repetition.

Also, are these paths expected to be relative to the cradle root directory?
What would happen in the face of nested multi-cradles, such as:

cradle:
  multi:
    - path: "appA"
      config:
        cradle:
          stack:
            - path: ./src
              component: appA:lib
              stackYaml: stack-8.8.3.yaml
    - path: "appB"
      config:
        cradle:
          stack:
            - path: ./src
              component: appB:lib
              stackYaml: stack-8.8.3.yaml

I guess in this case, the stackYaml needs to be relative to the cradleRootDir, otherwise we can not express the case that appA and appB have the same parent stack.yaml file.

jneira · 2020-07-31T16:34:34Z

Unfortunately stack shoud be a list or key-values.
My ideal shape would be

cradle:
  stack:
    config: stack-8.8.3.yaml
    components:
      - path: "./src/full"
        target: Agda:lib
      - path: "./src/main"
        target: Agda:exe:agda

But it would be out of scope of this pr...

WorldSEnder · 2020-07-31T18:01:19Z

Paths are supposed to be relative to the hie.yaml file, which also is the working directory of the stack command. I favor @jneira's config shape, and I suppose the following would work during parsing:

Look at the Value under stack:
If it's a list, do what has already been implemented in this PR.
If it's a an object with a components: key, parse those as a list of configurations, parse the rest of the object as a single configuration, then monoidally combine these configurations, where the more specific options overwrite the options in the parent one.
If there is no components:, parse it as a single configuration.

WorldSEnder · 2020-07-31T19:58:47Z

Also, are these paths expected to be relative to the cradle root directory?
What would happen in the face of nested multi-cradles, such as:
cradle:
  multi:
    - path: "appA"
      config:
        cradle:
          stack:
            - path: ./src
              component: appA:lib
              stackYaml: stack-8.8.3.yaml
    - path: "appB"
      config:
        cradle:
          stack:
            - path: ./src
              component: appB:lib
              stackYaml: stack-8.8.3.yaml
I guess in this case, the stackYaml needs to be relative to the cradleRootDir, otherwise we can not express the case that appA and appB have the same parent stack.yaml file.

I added a test case (multi-stack-with-yaml) with this config, where I needed to say -path: appA/src instead of ./src. Not sure if this is intended behaviour, have a look once I pushed. So it seems that your guess is already the behaviour implemented. To be concrete, the stackYaml is relative to the working directory, which is, as far as I can tell, the cradleRootDir.

cradle:
  multi:
    - path: "appA"
      config:
        cradle:
          stack:
            - path: appA/src
              component: appA:lib
              stackYaml: stack-8.8.3.yaml
    - path: "appB"
      config:
        cradle:
          stack:
            - path: appB/src
              component: appB:lib
              stackYaml: stack-8.8.3.yaml

WorldSEnder · 2020-07-31T20:29:45Z

I'm a bit lost as to why the 8.10.1 test cases are failing, after all. I use ghc 8.10.1 locally, too. Apart from this issue the test appear just fine. ~~Secondly, the Travis CI build seems to not find files, that the other builds do find, what is going on there?~~ Figured out I forgot to add files to Extra-Source-Files for travis to pick them up.

fendor

Everything looks pretty nice so far!
I dont have much time this week, but I plan to review it next weekend!

EDIT: should I forget to review till next week, feel free to ping me.

README.md

WorldSEnder · 2020-08-11T06:12:48Z

Ping @fendor

fendor

Sorry for the long wait!
Awesome work, I just have a few questions.

The config parser is way more permissive than it used to. While that is ok in principle, there are some oddities.

E.g. this configuration:

cradle:
  cabal:
    component: "exe:cabal"
    components:
      - path: "./src"
        component: "lib:hie-bios"
      - path: "./"
        component: "lib:hie-bios"
    components:
      - path: "./src"
        component: "exe:hie-bios"
      - path: "./"
        component: "exe:hie-bios"

is allowed and produces this config, which seems weird and wrong in my opinion:

Config
  { cradle =
      CradleConfig
        { cradleDependencies = []
        , cradleType =
            CabalMulti
              { defaultCabal = Cabal { component = Just "exe:cabal" }
              , subCabalComponents =
                  [ ( "./src" , Cabal { component = Just "exe:hie-bios" } )
                  , ( "./" , Cabal { component = Just "exe:hie-bios" } )
                  ]
              }
        }
  }

I don't think this should be allowed, even if it semi well-defined.

Other than that, LGTM. Just the parser is a bit too permissive in my opinion. But if you would like to tackle that in a follow up PR, I don't mind merging it.

fendor · 2020-08-18T15:45:17Z

src/HIE/Bios/Config.hs

-    | Stack { component :: Maybe String }
-    | StackMulti [ (FilePath, String) ]
+    = Cabal { cabalType :: !CabalType }
+    | CabalMulti { defaultCabal :: !CabalType, subCabalComponents :: [ (FilePath, CabalType) ] }


This also allows the config file:

cradle: cabal: components: - path: "./src" component: "lib:hie-bios" - path: "./" component: "lib:hie-bios"

While I dont expect any harm of it (and maybe we want soonish a similar field for cabal) but I just wanted to document it.

fendor · 2020-08-18T15:50:11Z

src/HIE/Bios/Config.hs

-  = fail "Not a valid Configuration type, following keys are allowed: component"
-parseStackOrCabal _ multiConstructor (Array x) = do
-  let parseOne e
+parseSingleOrMultiple single multiple parse = doParse where


Interestingly, this allows this configuration:

cradle: cabal: components: - path: "./src" component: "lib:hie-bios" - path: "./" component: "lib:hie-bios" components: - path: "./src" component: "exe:hie-bios" - path: "./" component: "exe:hie-bios"

with this result:

Config { cradle = CradleConfig { cradleDependencies = [] , cradleType = CabalMulti { defaultCabal = Cabal { component = Nothing } , subCabalComponents = [ ( "./src" , Cabal { component = Just "exe:hie-bios" } ) , ( "./" , Cabal { component = Just "exe:hie-bios" } ) ] } } }

That's a good catch, didn't think about multiple components entries. I'll fix this before the PR is merged.

Thinking about this a little bit more, isn't a duplicate key (components) an error according to the yaml spec?

Mapping nodes are somewhat tricky because their keys are unordered and must be unique.

In that case, I suppose this PR is relevant and https://github.com/mpickering/hie-bios/pull/230/files#diff-0478bef4574ee638b0c136cccc31479aR288 should use decodeFileWithWarnings

I added a test case that's supposed to fail on the config with duplicate components key.

lukel97 · 2020-08-18T17:57:24Z

This makes a lot of sense to have, what is the stackYaml path relative to? To me I would intuitively expect it to be relative to the hie.yaml file, not the component/cradle root

WorldSEnder · 2020-08-18T20:29:00Z

@bubba Maybe I'm missing something but I was basing the consideration on the implemented reality in multiCradle. The cur_dir is taken as the base path to append the component sub-paths cur_dir </> p off of. As I read it, this is relative to the cradle root, then. I would also agree with the suggestion to unambiguously make all paths inside hie.yaml relative to the file itself.

Additionally, it seems that the main entry point loadCradleWithOpts simply takes the directory containing hie.yaml as the cradle root directory, so it seems that relative to cradle root and relative to hie.yaml often means the same thing.

lukel97 · 2020-08-19T11:41:13Z

@WorldSEnder

I would also agree with the suggestion to unambiguously make all paths inside hie.yaml relative to the file itself.
This is probably worthy of a separate issue and discussion, but for now in this PR probably best then to just stick with making it relative to the cur_dir for consistency

fendor

LGTM, thank you!

src/HIE/Bios/Cradle.hs

fendor

LGTM!

Co-authored-by: fendor <[email protected]>

WorldSEnder added 2 commits July 31, 2020 16:27

allow specifying a stack.yaml for stack configurations

406c999

fix test cases

0e6e893

fendor reviewed Jul 31, 2020

View reviewed changes

WorldSEnder added 2 commits July 31, 2020 21:35

add test cases for stack with custom yaml

eff2a8b

add parser test cases

2eb14ea

whitespace

5c36ecb

WorldSEnder added 5 commits July 31, 2020 23:42

change suggestive name

de64b9b

fix test cases (again)

d67fd55

add missing files to distribution

ef228fb

fix typo

bdd9b2b

default settings for configurations

5702807

WorldSEnder marked this pull request as ready for review August 1, 2020 20:51

jneira mentioned this pull request Aug 1, 2020

Multiple stack.yaml files haskell/haskell-ide-engine#1799

Closed

ahri mentioned this pull request Aug 1, 2020

Support for multiple GHC versions via multiple stack.yaml files? haskell/haskell-language-server#268

Closed

fendor requested changes Aug 2, 2020

View reviewed changes

README.md Show resolved Hide resolved

WorldSEnder added 2 commits August 11, 2020 07:49

add warning about multiple stackYamls

98ad7f7

Merge remote-tracking branch 'origin/master' into master

ec61948

fendor approved these changes Aug 18, 2020

View reviewed changes

review fixes

9e1fa2f

WorldSEnder requested a review from fendor August 19, 2020 13:07

[ci skip] document changes to cabal configurations

05f4462

fendor approved these changes Aug 22, 2020

View reviewed changes

src/HIE/Bios/Cradle.hs Outdated Show resolved Hide resolved

fendor approved these changes Aug 22, 2020

View reviewed changes

Use suggested changes

8969b59

Co-authored-by: fendor <[email protected]>

fendor mentioned this pull request Aug 23, 2020

Prepare release 0.6.3 #238

Merged

fendor merged commit 0ed7604 into haskell:master Aug 23, 2020

fendor mentioned this pull request Aug 31, 2020

stack project in sub-directory #242

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow specifying a stack.yaml for stack configurations #230

Allow specifying a stack.yaml for stack configurations #230

WorldSEnder commented Jul 31, 2020 •

edited

Loading

fendor left a comment

jneira commented Jul 31, 2020 •

edited

Loading

WorldSEnder commented Jul 31, 2020

WorldSEnder commented Jul 31, 2020 •

edited

Loading

WorldSEnder commented Jul 31, 2020 •

edited

Loading

fendor left a comment •

edited

Loading

WorldSEnder commented Aug 11, 2020

fendor left a comment

fendor Aug 18, 2020

fendor Aug 18, 2020

WorldSEnder Aug 18, 2020

WorldSEnder Aug 18, 2020

WorldSEnder Aug 19, 2020

lukel97 commented Aug 18, 2020

WorldSEnder commented Aug 18, 2020

lukel97 commented Aug 19, 2020

fendor left a comment

fendor left a comment

Allow specifying a stack.yaml for stack configurations #230

Allow specifying a stack.yaml for stack configurations #230

Conversation

WorldSEnder commented Jul 31, 2020 • edited Loading

fendor left a comment

Choose a reason for hiding this comment

jneira commented Jul 31, 2020 • edited Loading

WorldSEnder commented Jul 31, 2020

WorldSEnder commented Jul 31, 2020 • edited Loading

WorldSEnder commented Jul 31, 2020 • edited Loading

fendor left a comment • edited Loading

Choose a reason for hiding this comment

WorldSEnder commented Aug 11, 2020

fendor left a comment

Choose a reason for hiding this comment

fendor Aug 18, 2020

Choose a reason for hiding this comment

fendor Aug 18, 2020

Choose a reason for hiding this comment

WorldSEnder Aug 18, 2020

Choose a reason for hiding this comment

WorldSEnder Aug 18, 2020

Choose a reason for hiding this comment

WorldSEnder Aug 19, 2020

Choose a reason for hiding this comment

lukel97 commented Aug 18, 2020

WorldSEnder commented Aug 18, 2020

lukel97 commented Aug 19, 2020

fendor left a comment

Choose a reason for hiding this comment

fendor left a comment

Choose a reason for hiding this comment

WorldSEnder commented Jul 31, 2020 •

edited

Loading

jneira commented Jul 31, 2020 •

edited

Loading

WorldSEnder commented Jul 31, 2020 •

edited

Loading

WorldSEnder commented Jul 31, 2020 •

edited

Loading

fendor left a comment •

edited

Loading