-
Notifications
You must be signed in to change notification settings - Fork 114
plugin alpha version #364
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
plugin alpha version #364
Conversation
Thank you @urbanjost . It works nicely on Windows 10. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
As @LKedward mentioned in #362, the current file structure prevents fpm(1) from being used as an fpm(1) package, and that would be easy to change. In addition to the reasons proposed I think for this fpm-NAME subpackage support to be most useful some of the independent apps that could result (such as a snapshot of fpm-search) could be distributed as part of the package with a pure fortran single-file bootstrap version of fpm. So another example might be the Someone might like to make an fpm-license command so you could enter "fpm license MIT" and create a LICENSE file. So if it were a package and you wanted to upgrade or get a specific version it could be a matter of and Haskell fpm (h-fpm) can still build packages with fpm.mk and CLI options for loading and linking that Fortran fpm (f-fpm) lacks So is it time to have the fledgling leave the nest and have an fpm package for fpm, as proposed? |
@urbanjost Are you available for this month's video call? It'd be good to discuss this PR broadly and I'd like to see it go forward. If not, and if you agree, I'd like to present and discuss this PR for you. |
Very unlikely I will make the meeting, so feel free to discuss it. A few thoughts that might be interesting but very preliminary -- I cloned the fpm repository partly to test fpm-search using an alternate repository but also because I was toying with the idea of making a "plugin" repository with (aware of the catch-22) fpm-search, maybe fpm-single, fpm-man (being what I called fman and fan), a bash shell to get and install a plugin assuming it is an fpm package (which is potentially very limiting) and looking at using "fpm build --show-model" as a way to simulate making plugins model aware to play with the concept. If fpm becomes a package soon that would be easier; but without a firmly defined interface that is just an experment.. So I see a richer support of plugins as potentially quite powerful. I think it could be used to develop features early that could be very system-dependent without polluting the core Fortran fpm functionality (you could have an fpm-cmd for Linux and for WIndows that did the same thing but used gfortran extensions or OS-specific commands more liberally than I would like to see fpm do directly (except via stdlib) and so on. fpm-search is by far the best example so far; but fpm-single is an indication there is more potential there. In any case no matter what the direction just allowing fpm-cmd to be called as "fpm cmd" gets the ball rolling. That is somewhat trivial by itself (you could of course just call the command "fpm-cmd") but with some kind of way of being able to query the model I have a alpha-alpha program called fpm-shell that lets you run the built-in commands in an "fpm shell" where you can call the subcommands without a prefix and plugins too that is an interesting plug-in. These are just rough ideas to play with at this point, and plugins are supported but do not seem to be a big component of other package managers; which is offsetting -- but supporting a modular model for fpm seems promising so I am behind anyone that wants to advocate for it. Maybe it will just allow for a simple way to prototype functions that will ultimately become core fpm commands but I think that will allow for faster fpm development too so go for it. Wondering if anyone has strong opinions on this before the meeting. That would be interesting to hear. |
Apologies for the delay with this PR; I am in support of this approach but haven't yet had a chance to look at it in detail and play around with it. Also, as noted above, fpm needs restructuring before plugins can properly access the model, but there are no fundamental blockers on why this can't be done now. I would advise against trying to parse the output of |
@urbanjost we discussed this PR on the call, you can see it here (relevant discussion at about 42:45). Overall there was support for this approach going forward, with some suggestion for improvement, such as not searching for plugin executable in the global PATH, but perhaps only in a specific directory that is not in the PATH. |
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 @urbanjost, with fpm now residing in the repo root, we can start to support prototyping and tinkering with plugins by using fpm as a dependency.
I have no major concerns with these changes, only a few minor comments.
Am I right in thinking that the which
function may return a path even for non-executable files? Not a major issue, but could cause some tricky issues.
The separator
function looks like a useful addition to avoid issues on cygwin/msys etc..
There was some discussion in the Monthly call about not searching the path but requiring a plugin manifest of sorts. I actually think the current implementation is fine as a starting point. IMO there shouldn't really be any security concerns here since your system would already be compromised if you can't trust an executable on your path.
I have a which(3f) function that uses ISO_C_BINDING interfaces to POSIX routines that checks execute permission, but did not have an equivalent for other systems so I simplified which(3f) so it just tests for file existence, which can be done with pure Fortran. I am assuming that stdlib will provide such a function in the future. I started a separate module in my private files called M_OS where I was experimenting with the reliability of separator, the reliability of testing for certain theoretically OS-specific files, environmental variables and other ways to emulate the uname(3f) POSIX function but have not had the time recently to pursue that, put in my own projects I have found that separator is useful of itself, particularly in MSWIndows where old tricks like testing for the directory ./ no longer work as POSIX and DOS filepaths are sometimes both supported. If fpm is assumed to require gfortran there are gfortran extensions that can be used to solve all those issues. Essentially, the M_OS module is just a module whose purpose is to determine OS-specific information (what file separator should I use, what OS am I using (letting me assume certain commands will be available, for example). Considering in the POSIX world I just call uname(1) it is surprisingly complex to just say "where am I" in a really robust way. So I am aware of the limitations but felt this was sufficient for an initial prototype. A plugin becomes potentially much more powerful if it can use fpm(1) as a package. This prototype is essentially laying some groundwork for exploring the possibilities |
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.
With two functions to check the OS we could get into a situation where get_os_type() == OS_WINDOWS
and separator() == "\"
disagree. I would prefer to have one function to handle the OS detection logic with a single point of failure rather than multiple different.
Can we merge the logic of separator
with get_os_type
to refine our OS detection logic and than use get_os_type
in separator
to return the correct one?
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.
Pending others' reviews, I support this going forward.
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.
@urbanjost This patch also enables the response file support from M_CLI2 if I have seen this correctly. Can we add a short sentence on this in the help printout as well? Something like:
@file
Read command-line arguments from file. The arguments read
are inserted in place of the original @file argument.
If file does not exist or cannot be read, then the argument
will be treated literally, and not removed. Each line in file
is interpreted as a separate argument. The file may itself contain
@file arguments. Any such argument will be processed recursively.
@urbanjost, I think there is a strong consensus to move this PR forward. Do you need help resolving the conflicts? |
Been gone for a bit. I cannot use github to resolve the conflicts or to look at them. Everything having to do with the conflicts appears to be grayed out. |
This is a reconstituted PR for an alpha version supporting plugins. previous comments are very valuable and should be looked at in closed PR #362. There were issues with rebasing the original PR to merge into the latest master and it was easier to just reintroduce the changes in the code.