Skip to content

Refactor to make it easier to start developing the command line aspects #151

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

Merged

Conversation

everythingfunctional
Copy link
Member

I'm proposing this as the start of the design for the command line aspects. I'm thinking I'll go ahead and start developing a command line parsing library, but the rest of the application doesn't need to know anything about it this way.

Unfortunately, this makes fpm incapable of building itself now. Should we start hard-coding some stuff to at least keep that working?

Copy link
Member

@certik certik left a comment

Choose a reason for hiding this comment

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

I've already hardcoded some stuff just so that fpm can build itself. If at all possible, I think it's helpful to preserve it, otherwise it might be quite hard in the future to get fpm to build itself.

My understanding is that the fix is quite simple --- it just needs to scan the modules and build them in correct order. So we should implement this feature.

Otherwise I think it's fine.

Yes, you should develop the command line parsing library, and we should integrate it; assuming none of the options at #135 work.


call get_command_line_settings(cmd_settings)

select type(cmd_settings)
Copy link
Member

Choose a reason for hiding this comment

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

Why cannot this just be an integer? It seems quite complicated this way with the class.

Copy link
Member Author

Choose a reason for hiding this comment

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

The different commands will have different options and arguments associated with them. This way the various operations are essentially completely independent.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of this style either, IMO it unnecessarily complicates the logic. You'll have as much boilerplate as with a procedural style. But it works so I'm not opposed to it.

Copy link
Member

@milancurcic milancurcic left a comment

Choose a reason for hiding this comment

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

I think this is a good refactor, thank you. What makes it not build with fpm, I don't see it? Whatever it is, I suggest let's hardcode what's needed and make it build with fpm before merging.

I also encourage you to write your own CLI library. The more the better and Fortran certainly needs more. For what we need, I think it'd be hard to beat FLAP (Stefano put a few years of work into it). Until we can evaluate libraries in practice, intrinsics will give us the bare bones needed to get us going.

@everythingfunctional
Copy link
Member Author

It's basically just not doing anything to figure what order it needs to build the modules. Hardcoding it would be a short term solution, but will very quickly become a mess.

@everythingfunctional
Copy link
Member Author

I looked more closely at FLAP. I really like the API, as it follows pretty closely the Python argparse library. However, it has a few limitations that I think rule it out.

  • It's licensed under GPLv3, which (I believe) is incompatible with fpm's MIT license
  • It doesn't support being built with fpm, and doesn't look very easy to convert (due to next issue)
  • It has several external dependencies that are managed as git submodules (a significant issue for bringing in as an fpm package)
  • It doesn't (seem to) support trailing, pass-through arguments (i.e. fpm run -- args to executable)

I'm going to try and implement a very similar API, and will probably look at it for inspiration, but I just don't think it is the right solution for fpm right now

@milancurcic
Copy link
Member

Okay, I think I understand now. It's Fortran fpm that is trying to build itself in CI.

I agree that we aim for this to work, but it seems to me a strange requirement to have right now. Fortran fpm conforms to the fpm package rules, but Fortran fpm does not have fpm build implemented to follow these rules. It seems to me like the wrong requirement to chase in this PR.

Instead, I propose that:

  • The Fortran fpm test in CI should be to simply build the example project (generated by Haskell fpm new).
  • To pass the test the above, we need to either expand Haskell's fpm new to add a bare-bones app/main.f90, or only build the module in Fortran fpm build

@certik does this seem like a reasonable compromise?

@milancurcic
Copy link
Member

Brad, we discussed this a bit in #135. License is a non-issue (FLAP is multi-licensed). Your other bullets are real issues. However, nothing's stopping us from forking the project and pulling in the external dependencies. It could all be fpm-ized in a day, but no point in doing so until we are sure it's the road we want to take. I think it's too early to tell.

@certik
Copy link
Member

certik commented Jul 31, 2020

It's fine I guess if it can't build itself. It currently runs some tests, and we should add a few more to test it a bit.

@milancurcic milancurcic self-requested a review August 1, 2020 01:02
Copy link
Member

@milancurcic milancurcic left a comment

Choose a reason for hiding this comment

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

IMO it's good to go.

@certik certik merged commit 39fb22d into fortran-lang:master Aug 1, 2020
@certik
Copy link
Member

certik commented Aug 1, 2020

I checked the tests, they seem to work. The Fortran FPM is tested on the simple hello world example, so I think we are in good shape and I merged it. I personally prefer more procedural approaches than the OO approach, but let's just try it, and we can refactor later if we decide in the future.

@urbanjost
Copy link
Contributor

I implimented the parsing using M_CLI2 and added enough (Posix-dependent) system routines to impliment the "new" subcommand in https://github.com/urbanjost/M_CLI2.git in the subdirectory fpm.cli. It also parses the other commands but does not do anything new with them other than echo the argument values to show the CLI parsing is working. Implimenting the "new" command was just to demonstrate the parsing, but if some can add equivalent routines for the mkdir,chdir,perror for other systems it would be portable, as the CLI interface is all Fortran. If this looks like it is sufficient details like completing the help text, etc. will be completed but I did not want to go much further if no concensus that this is sufficient is forthcoming. I have a basic Fortran-based utility for creating a basic Make file that could be used to do the builds (on systems with make) but not much need to do step two if step one is not going to work out. Could not get a git request to work so it is not a pull request, just some files to replace fpm/src. If good enough feel free to move it to the git repository.

@arjenmarkus
Copy link
Member

arjenmarkus commented Sep 1, 2020 via email

@milancurcic
Copy link
Member

Thanks @urbanjost! I think Brad is working on CLI parsing for fpm. @everythingfunctional, if you have any work done, can you push it to a branch on your fork so that @urbanjost can coordinate with you? If not, should we review @urbanjost's implementation?

@milancurcic
Copy link
Member

@arjenmarkus Do you know the status of MarDiehl/stdlib_os regarding Windows? We discussed this briefly on the last call and as I understood from @MarDiehl, Windows still needed some work. Is that correct?

@everythingfunctional
Copy link
Member Author

@milancurcic and @urbanjost , I'm currently working on a command line library that supports a more declarative style, and isn't hardwired to interrogating the real command line (i.e. calling get_command_argument). I haven't made a ton of progress yet, but you can check it out here.

The reason to do it this way, is it will make it possible to unit test the command line aspects, without needing external testing scripts, including how erroneous commands are handled. For example it becomes possible to write a unit test like, "if a name for the new package isn't passed to the new command, an error is reported", as well as "if the '--with-test' flag is passed to the new command, the returned settings object has the with_test component set to true", all without any connection to any I/O.

@milancurcic
Copy link
Member

That sounds good to me. IMO the CLI stuff is not as high priority as stdlib_os so we can take some time to carefully design it. In the interim, Fortran intrinsics can get us by.

@urbanjost
Copy link
Contributor

There are five command line parsers in my "General Purpose Fortran" repository. Like what you are proposing M_kracken can process arbitrary strings as well as the command line arguments, as it was was originally created as part of a Fortran-based shell and was not even used for command line parsing; M_CLI2 only does command line parsing by default, but was derived from a more general utility that allowed for the prototype parser to be called twice instead of calling the prototype parser and then the command line parser, but to truly test program execution the unit test program calls the test program recursively and does not need any external scripts to do do; albeit it builds a command stack and if that got big enough you could run out of process space. So you can write a Fortran code that tests the command line parser by actually calling it that does not require an external script. Between the M_system Posix interface and M_CLI/M_args/M_kracken/M_getopts/M_getopts_long/M_CLI2 command line crackers and M_io and M_debug/M_journal/M_msg unit testing modules and the regular expression routines and the make-file maker "makeout" all the parts except the toml reader and curl interface exist in that collection; except that the OS interface is POSIX-only so I have only used it in CygWin on a PC; but the general concensus here seemed to be to wait for the stdlib routines of the same or similar function to be developed so I did not see that an fpm using those would be acceptable. But M_CLI2 seemed a very good fit for the CLI interface module the Haskell fpm(1) model so I put together that code which I believe emulates all the current fpm(1) CLI interface plus allows for using -- instead of --args and allows for seperate help for each subcommand and I think is very easy to understand (usage requires one call per subcommand to set up the command options and parse the command line and then at a maxiumum a call to get_args for each keyword plus optional use of the array unnamed and string variable remaining for unassociated arguments on the command line.

So as an alternative you might look at the test program for the M_CLI2 module for how to test a command line parser by actually calling the program recursively.

Also, M_CLI2 has a routine in it for parsing a prototype string, but it does not behave exactly like calling the command line would (the string delimiter must be a double-quote, etc...).

We crossed paths apparently in that when I started that I did not see (or missed) that anyone was implementing a CLI interface and thought I would do it quickly enought that it would not needed announced until I had something working.
Murphy;s Law at work, I suppose.

@arjenmarkus
Copy link
Member

arjenmarkus commented Sep 2, 2020 via email

@everythingfunctional
Copy link
Member Author

@urbanjost , it sounds like I need to take some time to go and look at your libraries in more detail. If that all works we may be able to get the Fortran version of fpm working a lot faster than I thought.

@arjenmarkus
Copy link
Member

arjenmarkus commented Sep 3, 2020 via email

@urbanjost
Copy link
Contributor

@everythingfunctional. I put a simple CLI interface together with M_CLI2 to demonstrate an approach I think could be a good starting point. I did not include the code for making any of the other parts functional but all the command arguments are parsed and displayed in the unimplemented subcommands for demonstration purposes. Since it only takes a few lines in everything except the fpm_command_line.f90 file it could easily be replaced later, but it seems easier to me to discuss the CLI from the perspective of a prototype. Did you have a chance to look at any of the listed interfaces?

Now that the TOML interface is in I think we can get a version that works on local files pretty quickly and it would be good to have the CLI in place to be able to pass options to run and test and new.

Does anyone have any ideas on whether there are existing utilities that can be depended on for dependencies or are people envisioning something totally in fortran, perhaps starting with daglib?

Is libcurl common enough on non-POSIX systems to be looking at using the fortran-libcurl interface? I have my own preprocessor and used that to jazz up my $INCLUDE directive on Redhat 8 and CygWin and it has worked very nicely
(even the Fortran standard does not say the argument of an INCLUDE statement has to be a file on the current platform, but I do not know of one that lets you grab a file with https: or ftp: -- it is actually pretty handy).

The new subcommand would be a lot easier if there were a mkdir and a chdir function. Since gfortran has a lot of system extensions (as do most modern Fortran compilers if not all) having the arguments available would let that command be completely functional, at least up to the level of the Haskell fpm(1). Is if fair to use the extensions (assuming a CLI is in place, as most of new requires parameters to be implimented). I thought I saw were someone was working on new but cannot find it; but I am sure it would need CLI parameters.

@everythingfunctional everythingfunctional deleted the refactor_for_command_line branch October 12, 2020 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants