-
Notifications
You must be signed in to change notification settings - Fork 108
alpha plugins, take III #484
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approved the original PR, and this all looks good to me 👍. I will however second Sebastian's previous comment about documenting the response file functionality which would be good.
The response file concept was part of an earlier PR proposing a --flag option which was reduced to just the --compiler option and a suggestion from @awvwgk. It allowed me to experiment with several concepts concerning an ealier --profile proposal and is useful but I decided at the time that it was probably better to put the profiles into the manifest itself with a compiler and os and flag list comprising a profile, and then (since the manifest file has no conditional capabilities) that fpm could then just choose based on current compiler and OS and profile name so the logic incorporated into fpm would not be too complex. As-is it does not quite do what I want with a subcommand, is not recursive, only uses the last specification of an option specified multiple times, which I originally planned on changing (and a few other things) and can select between options based on OS but not on compiler it has limitations but is useful so I put a basic description in of the simplest use case with a reference to the M_CLI2 github repository. I originally proposed it as a prototype for discussion but with the other solutions for a package-able custom build proceeding and being part of the GSOC and this being a "free" feature from M_CLI2 I think it can proceed as a supplemental feature as-is(?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this really looks great now.
Including the original PR (#364), we have four approvals so I'll now merge. Thanks @urbanjost |
When trying to update #364 all I got was "Sorry, an error occurred" so this is a reconstituted pull request. See #364 and #362 for previous comments.