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

Use less permissive types in the config monoids #2267

Open
sjakobi opened this Issue Jun 13, 2016 · 12 comments

Comments

Projects
None yet
3 participants
@sjakobi
Contributor

sjakobi commented Jun 13, 2016

ConfigMonoid (and probably other config monoids too) contains a few fields that have more permissive types than necessary. This has the effect that bad inputs go undetected or result in errors that are reported out of context or only after the inputs have been passed to ghc.

A few of the problematic fields:

  • configMonoidWorkDir
  • configMonoidOs
  • configMonoidExtraIncludeDirs

Example interaction:

$ stack --extra-lib-dirs --oops build
these-0.7: unregistering (local file changes: Control/Monad/Chronicle.hs Control/Monad/Chronicle/Class.hs Control/Monad/Trans/Chronicle.hs Data...)
these-0.7: configure
Configuring these-0.7...
Warning: 'extra-lib-dirs: --oops' directory does not exist.
...

Compare this to the error message for an argument that must be a Path Abs Dir:

$ stack --stack-root --oops path
option --stack-root: Failed to parse absolute path to directory: '--oops'

Usage: stack [--help] [--version] [--numeric-version] [--hpack-numeric-version]
...
@chrisdone

This comment has been minimized.

Member

chrisdone commented Jun 15, 2016

Agreed, do you want to submit a PR for this?

@sjakobi sjakobi self-assigned this Jun 15, 2016

@sjakobi

This comment has been minimized.

Contributor

sjakobi commented Jun 15, 2016

Agreed, do you want to submit a PR for this?

Yes I've already started to work on this.

One thing that would be useful for this, are FromJSON instances for the four Path types. I assume you wouldn't want these in path itself.
Should I make a path-aeson-instances or even a path-instances package?

@chrisdone

This comment has been minimized.

Member

chrisdone commented Jun 15, 2016

I suppose I wouldn't mind including instances for FromJSON in path itself. It should have four instances, one for each possible path combination. I'll merge a PR to path which includes FromJSON and ToJSON instances.

@sjakobi

This comment has been minimized.

Contributor

sjakobi commented Jun 15, 2016

I suppose I wouldn't mind including instances for FromJSON in path itself.

Great! I'll make a PR.

@chrisdone

This comment has been minimized.

Member

chrisdone commented Jun 15, 2016

Here are some necessary ingredients to achieve what you'd like for e.g. extra-lib-dirs:

It may eventually be a nice idea to include such an optparse combinator in the path package itself eventually, but for now it can be put in Options.hs.

Do you have everything you need? ☝️ 😄

@sjakobi

This comment has been minimized.

Contributor

sjakobi commented Jun 15, 2016

Do you have everything you need?

Thanks! :)

I'll ask if need anything else!

@sjakobi

This comment has been minimized.

Contributor

sjakobi commented Jun 19, 2016

I'd like to use better types in Urls and UrlsMonoid but I'm not sure what to pick:

@chrisdone

This comment has been minimized.

Member

chrisdone commented Jun 20, 2016

Possibly uri-bytestring?

@sjakobi

This comment has been minimized.

Contributor

sjakobi commented Jun 20, 2016

Possibly uri-bytestring?

Thanks, I didn't known about that one. One downside here is that it will accept any scheme, not just http or https.

I see that we later usually call Network.HTTP.Client.parseUrl on the URLs, so was wondering if we could just use Network.HTTP.Client.Request to encode the URL. But then you can't easily append a relative path to the URL as we do when creating build plan URLs…

I've also come across path-extra and urlpath but these don't seem to have any parsing utilities.

I guess I'll just make a newtype of URI.Bytestring.URIRef Absolute and make sure that it has the right scheme in the parser.

@sjakobi

This comment has been minimized.

Contributor

sjakobi commented Jul 21, 2016

I guess I'll just make a newtype of URI.Bytestring.URIRef Absolute and make sure that it has the right scheme in the parser.

I did that (this is pre-http-client-0.5) but I wasn't so happy with the result. (Can't really put my finger on what exactly though)

Now I'm thinking of something like this:

data Schema = Http | Https

data HttpUrl loc =
  { httpUrlSchema :: Schema
  , httpUrlHost :: Text
  , httpUrlPort :: Maybe Word
  , httpUrlPath :: Path Abs loc
  }

appendPath :: HttpUrl Dir -> Path Rel loc -> HttpUrl loc

httpUrlRequest :: (MonadThrow m) => HttpUrl File -> m Request

I believe this would fit the need of Stack.Types.Urls nicely.

I'm probably overthinking this. If someone feels like bikeshedding this, please help! ;)

sjakobi added a commit that referenced this issue Jul 22, 2016

Merge pull request #2294 from sjakobi/2267-less-permissive-config-mon…
…oids

Use less permissive types in the config monoids (#2267)
@Blaisorblade

This comment has been minimized.

Collaborator

Blaisorblade commented Aug 9, 2016

Coming from Java, I'd add File to Schema and use such URLs for things like #2232.

Also, your httpUrlRequest seems designed to not download a file from http://host/dir/, which is legal, and doesn't allow for a query string IIUC. But more importantly, I'm probably being naive but having to design your own URL type seems (a) hard (though it's not crypto) and (b) should seriously be unnecessary. But I should take a close look at available options.

@Blaisorblade

This comment has been minimized.

Collaborator

Blaisorblade commented Aug 13, 2016

I guess I'll just make a newtype of URI.Bytestring.URIRef Absolute and make sure that it has the right scheme in the parser.

I did that (this is pre-http-client-0.5) but I wasn't so happy with the result. (Can't really put my finger on what exactly though)

I've looked at your code and frankly it mostly LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment