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

Add a config option to override path and add options to each PROG #3118

Closed

Conversation

kgadek
Copy link

@kgadek kgadek commented Apr 10, 2017

This allows one to specify with-ld: and with-strip: in stack.yaml. Ref #2369

Ripoff #2264 from @khanage, includes subsequent changes in 113c637.

While it ignores the existence of #652 and only covers ld and strip, I would propose to include this anyway, as it's consistent with existing code.

Main motivation is the https://www.reddit.com/r/haskell/comments/63y43y/liked_linking_3x_faster_with_gold_link_10x_faster/ post — this patch should allow people to compare the compilation times using clang.

Checklist:

  • [*] Any changes that could be relevant to users have been recorded in the ChangeLog.md
  • [*] The documentation has been updated, if necessary.

This change is Reviewable

@kgadek kgadek changed the title Add a config option to override ld and strip path Add a config option to override path and add options to each PROG Apr 16, 2017
@kgadek
Copy link
Author

kgadek commented Apr 16, 2017

The following commit is a more comprehensive solution. A lot of similar code there, but does the job.

@mgsloan
Copy link
Contributor

mgsloan commented Apr 23, 2017

Wow, thanks for working on this! Sorry for the delay on reviewing it.

  1. Beyond supplying these arguments to configure, we will also want to use these in stack's invocations of ghc / ghcjs (building Setup.hs, ghci), ghc-pkg, ghcjs-pkg, and haddock. I think that covers it, but there may be more.

  2. Might also consider leaving out the uhc, lhc and jhc stuff as stack doesn't support them.

  3. These are going to really clutter up the help documentation. I think it'd be best to hide them by default, and have something similar to cabal's doc

    --with-PROG=PATH                 give the path to PROG
    --PROG-option=OPT                give an extra option to PROG (no need to
                                     quote options containing spaces)
    --PROG-options=OPTS              give extra options to PROG

Unfortunately this is not so straightforward with optparse-applicative, but once pcapriotti/optparse-applicative#253 is merged, something like this should be possible. I don't think it makes sense to delay this PR on that point.

It'd also be cool to avoid adding so many fields to the configuration type, possibly by having a datatype for programs and having Map Program [Text] and similar? Not really required, can be deferred to a later change.

@kgadek
Copy link
Author

kgadek commented Apr 28, 2017

  1. oh right. I'll ripgrep for usages
  2. makes sense. I'll also make a note about it in the docs
  3. 100% agree, I didn't know what to propose as the proper solution. I can very well add dummies --with-PROG & --PROG-option, and hide all of the new options… though obviously it's a hack. For the record, while the Add a new modifier for editing an option description pcapriotti/optparse-applicative#253 is merged, it's not a part of the release (at very least lts-8.5 --> optparse-applicative 0.13.2.0 doesn't have that). I think we could go the hacky-way and after merge make a ticket to fix that ugliness once Add a new modifier for editing an option description pcapriotti/optparse-applicative#253 is available.

The Map Program [Text] approach is okay. I was thinking about making a separate data type for this, but Map works fine.

As a related note, src/Stack/Options/ConfigParser.hs:configOptsParser is on the edge of getting obscure bugs (e.g. flip any of the *Path params) and being just unmaintainable. I don't know optparse-applicative enough to propose any solution to that. Ideas?

@kgadek kgadek force-pushed the 2369_override_strip_ld_path branch 2 times, most recently from a8dfb55 to 35a2db4 Compare May 7, 2017 15:11
@kgadek
Copy link
Author

kgadek commented May 7, 2017

  1. I don't know how to properly trace all places where I'd need to put the config options. Could you help me with this?
  2. Hmm, is there any other place in documentation that I shall update?
  3. Done (the hacky way). Shall I made some additional behaviour to exit when someone calls this dummy option (stack --with-PROG foo) by accident?
  4. Used maps. I'm not sure if I like Map ProgName (First (Path Abs File)) though. WDYT?
  5. Rebased & cleaned up commit history a bit.

@kgadek kgadek force-pushed the 2369_override_strip_ld_path branch from 35a2db4 to f4bf018 Compare May 7, 2017 16:48
@decentral1se
Copy link
Member

decentral1se commented Aug 7, 2017

Oh damn, it's a real pity to see this rotting 💣

Hmm, is there any other place in documentation that I shall update?

How about I take a stab at updating that, while you cover the rest.

Shall I made some additional behaviour to exit when someone calls this dummy option (stack --with-PROG foo) by accident?

Probably a good idea to include some nice message for this case.

From #3118 (comment):

  1. I don't know how to properly trace all places where I'd need to put the config options. Could you help me with this?
  2. Used maps. I'm not sure if I like Map ProgName (First (Path Abs File)) though. WDYT?

@mgsloan, any quick ones to get @kgadek on the right direction?

@mgsloan
Copy link
Contributor

mgsloan commented Nov 27, 2017

Sorry!! I don't have any good explanation of why I hadn't answered the earlier inquiries, other than misplacing browser tabs and having lots of other stuff to contend with.

One reason for this is that I am not so keen on having so much boilerplate. Maybe this is what Cabal does, but I would really prefer something that either:

  1. Has a list of programs at runtime, generates all the options etc from that, doesn't have separate fields.

  2. Alternatively, uses TH to generate the code.

Maybe that's overkill, but it'd be nice if adding a program to the list could be a modification in one or two spots.

I don't know how to properly trace all places where I'd need to put the config options. Could you help me with this?

Anything that invokes processes. Search for Process. In particular, the stuff in System.Process.* modules.

This is part of the idea with making this more of a runtime thing rather than having separate code paths for each overidable command. The code in System.Process.* would check if the location of a particular command name has been overridden, and instead use that. To ensure that this matches, up, can have a constant like ghcProgramName = "ghc" that is used in both spots.

Done (the hacky way). Shall I made some additional behaviour to exit when someone calls this dummy option (stack --with-PROG foo) by accident?

Seems reasonable to check that.

Used maps. I'm not sure if I like Map ProgName (First (Path Abs File)) though. WDYT?

Seems alright. I hope there is a good way to do (String -> Maybe ProgName). If there isn't, then there won't be a convenient way to do the Process code. Other than perhaps listing what is accepted for the with options, I feel like cabal's ProgName sum type is overkill. Might be nice to use it so that exhaustiveness checks tell us when it's been updated. However, I think newtype ProgName = ProgName String would be just as good and a lot less boilerplate.

Anyway, apologies again @kgadek your good intentions here are appreciated. I hope this hasn't put you off doing other contributions. I really encourage it! Could certainly rebase and implement the stuff described above. Feel free to close this one out and start a new PR as well if that would be appealing.

Will try to be more on the ball in the future, should be possible. And feel free to ping multiple times if we're lagging on review.

@mgsloan
Copy link
Contributor

mgsloan commented Nov 27, 2017

Ah, perhaps something like ProgName is good. Could have it actually be directly provided to the System.Read.* functions and other process functions. Or, perhaps a layer atop.

The idea is that if TH is used to generate the options, then adding a constructor to a ProgName datatype will automatically cause there to be a --with-PROG option for that new program.

@snoyberg
Copy link
Contributor

It looks like this has stalled, and has begun accumulating merge conflicts. I'm going to close out for now. @kgadek if you're still interested in pushing forward with this, please drop a comment and we can figure out how to get this to continue.

@snoyberg snoyberg closed this Jun 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants