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

Setup-info configurability #5031

Merged
merged 14 commits into from
Nov 24, 2019
Merged

Setup-info configurability #5031

merged 14 commits into from
Nov 24, 2019

Conversation

NightRa
Copy link
Contributor

@NightRa NightRa commented Sep 13, 2019

This PR comes to fix various issues related to how stack decides where to resolves it's tooling locations, for the initial purpose of using stack offline or behind a firewall.

  • See the documentation changes and the changelog for details on the new behaviors.

Stack 2 broke the previous behavior of the setup-info field - which now does not overwrite the default location. #2983, #2913

This existing breakage allowed for a backwards-compatible implementation of the wanted behavior - via the setup-info-locations yaml config for remote locations, and using setup-info for inline configuration only.

Also, as the --stack-setup-yaml CmdArg has been deprecated for 3 years, it is now removed, in favor of --setup-info-yaml #2647

Design decisions:

  1. Usability / Backwards compatability: The default stack-setup-2.yaml is added to the setup-info-locations only if no locations were specified (either in the cmd args or in the config yaml)
  2. Extendability: Allow extending setup locations - inline configs extend the default, and locations allow extensions by the locations following them.

PR considerations:

  • Documentation was updated
  • Changelog was updated
  • Manual tests performed:
    • Overriding precedence between setup-info, command line args, setup-info-locations and the default location, with missing / invalid locations to check that no unwanted accesses are made.

Relevant issues:

Copy link
Contributor

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

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

A few comments about the design, but overall this looks good, thank you!

If you need to **replace** it, use the `stack --setup-info-yaml` command-line
argument instead. The default setup metadata is in
[stack-setup-2.yaml](https://github.com/commercialhaskell/stackage-content/raw/master/stack/stack-setup-2.yaml).
Note that specifying this config *does not* the default `stack-setup-2.yaml` from being consulted as a fallback.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's a typo in this sentence, I'm not sure how it's supposed to read.

<*> many (
strOption
( long "setup-info-yaml"
<> help "Alternate URL or absolute path for stack dependencies"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would think a relative path would be even more convenient and recommended, so that the YAML file can be included in a repo and the code will work on multiple machines. Any underlying reason for preferring absolute paths?

@NightRa
Copy link
Contributor Author

NightRa commented Nov 23, 2019

@snoyberg
Fixed PR comments & Implemented relative-path setup-info yaml files & tool paths (ghc & 7z).

There are 2 outstanding issues which are out-of-scope for the current PR and should be handled seperately:

  1. #5095 - when used outside of a project, global-project/stack.yaml configs (which are not project configs) are ignored, which shouldn't be the case.
  2. When specifying a relative setup-info-locations path in %STACK_ROOT%/config.yaml, it's relative to the current project - either the local project or the global project, which is counter-intuitive, and should be relative to the config.yaml file - so only urls or absolute paths should be used in config.yaml for now.

@NightRa
Copy link
Contributor Author

NightRa commented Nov 23, 2019

Also, feel free to re-arrange the change-log, I had no clue which sections the changes should be in.

@snoyberg snoyberg merged commit 02d36f1 into commercialhaskell:master Nov 24, 2019
@snoyberg
Copy link
Contributor

Thanks!

@NightRa NightRa deleted the feature/setup-info branch April 15, 2020 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants