CLI interface to further development of subcommands #182

urbanjost · 2020-09-20T19:51:46Z

CLI interface to further development of subcommands

pass settings

extended help for each subcommand

change commit= to ver= in fpm.toml

ver= does not work either

no specific version as ver= does not work for M_CLI2

add test program for CLI

fix fpm.toml version reference

remove --usage references from help text

comment and clarify CLI unit test

basic RUN subcommand

restore fpm_command_line.f90 changes

remove non-zero STOP for no parameters for testing

spelling error in help

use basename to make sure name is a simple name

remove dash from executable name to see if it clears MSWindows build error

try one more like previous build to clear error

one more time like previous version to see if build error clears on MSWindows

debug run to see PC variables

make quoting of -- ARGS values less platform dependent and change test accordingly

change .gitignore

RESTORE

RESTORE FROM BACKUP

urbanjost · 2020-09-20T20:02:40Z

This should implement the CLI; with the only reference to which parser (M_CLI2) used is isolated to the fpm_command_line.f90 file. I ran into what I think is a bug with allocatable character variables of allocatable length but I believe just fixing the length to 4096 resolves that with gfortran, and implements parsing equivalent to all the parameters of the Haskell fpm(1). It should be easy to switch to an alternate parser by changing only fpm_command_line.f90

I ran into a problem with what appears to be the Fortran TOML interface not recognizing commit= on a dependency as well, which I think shoud be changed in the TOML interface?

With this in place it would be a lot easier to move forward on new,run,test for at least a simple module with no dependencies although the "libcurl" and "libgit" and "os_system" interfaces are more significant it is hard to try things out without the CLI interface.

certik · 2020-09-21T15:14:55Z

Thanks!

The CI tests fail due to #184. We need to fix that.

everythingfunctional · 2020-09-21T20:41:20Z

I'm still a bit unclear on how exactly to use M_CLI2 (let alone how it works), and it kind of looks like the subcommand aspect isn't a very natural fit (that part is done without the use of M_CLI2).

But, since it's working, and is very well isolated from the rest of the code (which is what I was aiming for no matter what solution we used), I'm happy with it.

everythingfunctional · 2020-09-21T20:58:43Z

Does this just need to be rebased after #185 to fix the CI? I think that's what it looks like.

fpm/fpm.toml

LKedward

Thanks @urbanjost, I think this is good to get a working CLI in order.
I've played around with it, and all seems to work as expected. I really like the neat man-style help texts complete with examples!

The --usage option looks more like a helpful developer debug option than a user option.

Please see my comment regarding rev (not ver) keyword for the M_CLI2 dependency.
Otherwise this looks good to me 👍

New commits

urbanjost · 2020-09-22T13:13:49Z

OK. Make change to fpm.toml file and added a test of the CLI interface, which gets a little complicated if you want it to actually call a process; so it uses NAMELIST groups to write information from the subprocess back to the original test process but if you get past the NAMELIST usage it should be a thorough test.

M_CLI2 is designed with a more functional/procedural approach. The documentation on the web site or included in the manpages that come with and around 10 tests on the README hopefully give an idea of how it is more typically used; I tend to go procedural until I have a reason not too so I had a bit of a time figuring out how to best fit it into the model. Never used an ABSTRACT type before, for example. It is totally isolated and in a single module as you said, so it gets things going and should be easy to change out if anyone wants to. Parts of M_CLI2 have been modernized but it was originally written before F90 existed so it might look odd to someone approaching things from an OOP stance. The biggest oddity is a rather primitive dictionary so it could be split out to be stand-alone and the original dictionary code was overkill for just command parsing.

everythingfunctional · 2020-09-22T16:24:09Z

Thank you for adding the tests. Although I still find it quite hard to follow, what with calling itself recursively and using a file to pass data back. It's hard to see how the given command line corresponds to the expected settings object, or what the expected error is.

urbanjost · 2020-09-23T22:55:49Z

firefox cannot look at the failure; it displays everything off-screen on my machine; and a tool for doing pull requests is not doing what the documentation indicates it should so I cannot view this to confirm it. On my machines everything works fine. I have some simpler changes to suggest for run, test, and new that I would like to add but they depend on a functioning CLI. Any help on reviewing this so it can be included or deleted would be appreciated, but at least on my machine I can create a new repository, build, test and run it as long as it is as simple as when the new subcommand produces (no recursive build and build always builds, but actually at least partially functional; but it runs and tests all 22 of the test cases I have so I am looking forward to being able to contribute this so the bigger hurdles of downloading and building complex cases (building only what needs rebuilt) have a full foundation to be tested with.

certik · 2020-09-23T23:07:09Z

The Windows error is:

2020-09-23T13:23:55.0047154Z # gfortran (for build/gfortran_debug/app/main.o)
2020-09-23T13:23:55.1149736Z # gfortran (for build/gfortran_debug/app/fpm.exe)
2020-09-23T13:23:55.4213275Z fpm: Error when running Shake build system:
2020-09-23T13:23:55.4214301Z   at want, called at src\Build.hs:114:11 in fpm-0.1.0.0-9a29EtwrI5D7C1C0BiAJeV:Build
2020-09-23T13:23:55.4215120Z * Depends on: build/gfortran_debug/test/main/fpm-test.exe
2020-09-23T13:23:55.4216177Z   at need, called at src\Build.hs:120:13 in fpm-0.1.0.0-9a29EtwrI5D7C1C0BiAJeV:Build
2020-09-23T13:23:55.4217038Z * Depends on: build/gfortran_debug/test/main/test_manifest.o
2020-09-23T13:23:55.4217930Z   at &%>, called at src\Build.hs:137:11 in fpm-0.1.0.0-9a29EtwrI5D7C1C0BiAJeV:Build
2020-09-23T13:23:55.4220034Z * Depends on: build/gfortran_debug/test/main/test_manifest.o build/gfortran_debug/test/main/test_manifest.mod
2020-09-23T13:23:55.4221105Z   at undefined, called at src\Build.hs:142:25 in fpm-0.1.0.0-9a29EtwrI5D7C1C0BiAJeV:Build
2020-09-23T13:23:55.4221924Z   at error, called at libraries\base\GHC\Err.hs:78:14 in base:GHC.Err
2020-09-23T13:23:55.4222481Z * Raised the exception:
2020-09-23T13:23:55.4222946Z Prelude.undefined

Which seems like a bug in our Haskell fpm? @everythingfunctional do you know what the issue is?

urbanjost · 2020-09-24T00:52:02Z

Thanks. Cannot think of anything different that would trigger that that was not in one that passed. making some trivial changes to see if a new pull request refreshes something

LKedward

Thanks @urbanjost. I think this is good for the time being. Changes are self-contained to fpm_command_line.f90 and can be improved upon where necessary in future pull requests.

It is a shame that the existing test framework could not be leveraged here and I agree with @everythingfunctional that the CLI test program is difficult to understand.
Again I think this is something that can be addressed later; I would be happy to have a go at transferring your tests to the existing test framework.

I'm really not sure why the Windows CI is failing - it looks like fpm isn't able to build the multi-file test and I don't think we have any demo packages with multi-file tests.

fpm/src/fpm_command_line.f90

LKedward · 2020-09-24T07:48:07Z

fpm/src/fpm_command_line.f90

+
+        ! text for --version switch,
+        version_text = [character(len=80) :: &
+           &  'VERSION:     0.0.0, 20200920',                               &


Suggested change

& 'VERSION: 0.0.0, 20200920', &

& 'VERSION: 0.0.0, Pre-alpha', &

This may be better to avoid workload of updating the date at each pull request.

This is actually conflicting with the version specified in the fpm.toml, which says it is 0.1.0:

fpm/fpm/fpm.toml

Lines 1 to 3 in 973a2cb

name = "fpm"

version = "0.1.0"

license = "MIT"

Good spot @awvwgk . @urbanjost, would you mind matching fpm.toml here with instead:

'VERSION: 0.1.0, Pre-alpha'

urbanjost · 2020-09-24T13:46:10Z

A test of a command line parser that does not actually use the command line interface would be incomplete. I could just call the CLI parser directly but that would not check the use of the intrinsics for getting the command line arguments which can be quite different even in a single programming environment. So I think it is a much better test to actually call a program to test a program command line parser but I could be argued out of that.

If this is going to
work I doubt any single person will understand all the parts without this becoming a single-man project!! I found the basic layout quite alien at first myself; you have to get out of your comfort zone on collaborative stuff has been my experience; so if anyone has any specific questions about the test I would be glad to answer them or add comments to the test or change the test. It is not so much complicated as uncommon to use Fortran as a shell language; but I use Fortran that way all the time so it of course seems more intuitive. Just think of the calling program as being a shell language being used to call another program that just echoes its parameters out for you to read and verify. That makes it non-recursive. Now, one of the easiest file formats to write group of values out with is NAMELIST, where it takes one line to read and write the whole group, so the test program writes out what was passed to it as a NAMELIST file. That could be anything --- JSON, TOML, etc. but those are not built in to Fortran.
So now the "shell" (the parent call to the program) just calls the program a bunch of times with different parameters and checks what the program sees after being passed through the system. If the values match it is doing what is expected. That allows you to check every major combination of the parameters the fpm program has virtually directly in about 100 lines so I stand by the approach even though I know it is different.

I would find fpm unuseable if it can only handle one test file. I have projects that require hundreds, especially ones that have many modules in them so I hope that is not a limitation.

LKedward · 2020-09-24T14:48:49Z

A test of a command line parser that does not actually use the command line interface would be incomplete. I could just call the CLI parser directly but that would not check the use of the intrinsics for getting the command line arguments which can be quite different even in a single programming environment.

This is a fair point however one of the advantages of separating functionality into packages is that testing can be separated along the same lines. For example M_CLI2 is responsible for defining an abstraction that interfaces with get_command_argument intrinsics etc.; therefore testing at this level should occur within the M_CLI2 package (I assume it already does) and packages using M_CLI2 need only test what they implement atop M_CLI2. This ideally takes the form of 'short-circuiting' the get_command_argument intrinsic.

I don't really object to the recursive execution with namelist file, this is a neat end-to-end testing solution; my comment was referring to why this couldn't be implemented within the existing test framework like test_manifest.f90, test_toml.f90 and test_source_parsing.f90. I believe it is possible, but with some complication regarding where the existing parse() method will go. The existing test framework nicely defines test suites with self-contained tests that are easy to interpret.

I would find fpm unuseable if it can only handle one test file. I have projects that require hundreds, especially ones that have many modules in them so I hope that is not a limitation.

This is not the intended behaviour for fpm as has been discussed in #164. This may be a bug or unimplemented feature in bootstrap fpm.

everythingfunctional · 2020-09-24T16:07:55Z

The bootstrap fpm should support multiple test executables, I have multiple in order to test the IO parts of iso_varying_string. I'll see if I can spare some time today to look more closely at the failure. It seems it must be something Windows specific, since the other OS's are passing.

everythingfunctional · 2020-09-24T17:28:24Z

As I suspected, Windows paths were mucking things up again. I fixed that, and now it looks like there are legitimately a couple failing tests in the CLI on Windows.

everythingfunctional · 2020-09-24T17:39:20Z

It looks like something having to do with the way Windows deals with spaces in command line arguments being ... problematic. I think you have to use double quotes (IIRC).

See this question I once asked on Stack Overflow for some additional background and to start you down the rabbit hole: https://stackoverflow.com/questions/43813677/escaping-spaces-in-windows

everythingfunctional · 2020-09-24T19:01:24Z

It looks like you clobbered my build fix. Would you like me to re-push it?

urbanjost · 2020-09-24T19:20:22Z

Seems like it is having trouble with the Shake system again. I can remove the test and put the files back in the top directory as I think the problem in test17 is likely due with the way quoting is done on MSWindows or arguments are passed to an executable and unfortunately I do not have a programming environment on an MSWindows machine; I have access to a CygWin environment but it ran there. Using a PR to debug is a bit tough, especially as the most I can get to display in Firefox on Redhat is three lines at a time of the log file. This is getting too slow to proceed at this point. Disappointing, as I think we are well within the reach of having a functional Fortran fpm sans the network-related functionality.

urbanjost · 2020-09-24T19:25:38Z

Did not see the comment. Yes, I did not release pushing out another version would clobber your changes. Not sure what I do to avoid that as I thought your change was independent of this PR. I am quite curious what a debug run with the changes I just added would show, as not that familiar with MSWindows as a programming platform. This is the kind of stuff the test was intended to find, even if I did not expect any (M_CLI2 is a subset of code that has been used for years in hundreds of codes on many Unix and Linux systems. That is why you test I guess :>.

everythingfunctional · 2020-09-24T19:47:40Z

A force push generally means you're about to clobber something. In the case that someone has made changes on the branch, general practice is to do git pull --rebase before git push. I'll get it back.

urbanjost · 2020-09-24T19:54:00Z

Read the stack overflow descriptions. I think the headache will pass; but I might have to tweek M_CLI2 to detect the OS; just having trouble figuring out how it will work in Powershell versus MSWindows CLI mode versus called from a .bat file, etc. Thanks for the link. It would have been particularly confusing without knowing MSWindows was confused in the first place.

everythingfunctional · 2020-09-24T20:16:36Z

No problem. We're super close.

urbanjost · 2020-09-25T00:58:36Z

I have been using a line-mode command called git-pull-request that a friend swears by, and the --force appears to be built in so I need to figure out how to do this manually; the MSWindows quoting issue is bizarre. I may have to build a programming environment on an MSWindows machine to see if I can sort it out. I got diffferent issues with quoting using simple scripts in PowerShell and a cmd window with a .bat file and from calling a program I built in Cygwin from a cmd window (works fine in CygWin window). And having problems with the website from my browser. It looks like test15 worked which if the code I think is pushed there is there should be having the same issue. Really hard to look at. That is an outlier case I would like to just comment out and solve as a seperate issue. Wondering if the PC version of the Haskell version has any issues with arguments with spaces in an --args option and if not that might be a good clue. Can you just comment those tests out for now if that is simple? I would really like to get this moving and work on the changes in the "NEW" PR and get a basic functioning fortran version for simple local cases available and have something I think everyone case work with in that "NEW" PR.

everythingfunctional · 2020-09-25T01:12:16Z

Ok, that should fix it. I'm good with going ahead and merging this and opening an issue to deal with the Windows side later. I'll give everybody else a day to veto that before actually doing so though.

urbanjost · 2020-09-25T02:04:34Z

Great. Thanks!

awvwgk · 2020-09-25T06:27:30Z

Sorry, late to the party. I might be able to give a hint on the Windows issue. You can't escape using single quotes on the command line on Windows. Both failing tests, 13 and 17 are escaping their whitespace with single quotation marks / apostrophes, if I'm not mistaken.

awvwgk · 2020-09-25T07:33:47Z

It does indeed work, see: e0f9499 and the corresponding Windows test:

START:  TEST 17 CMD=test proj1 p2 project3 --release -- arg1 -x "and a long one"
 arguments seen directly by program
0[test]
0[proj1]
0[p2]
0[project3]
0[--release]
0[--]
0[arg1]
0[-x]
0[and a long one]

Note, j is the counter for the loop and i used uninitialized here, therefore we get zeros instead of useful numbers:

fpm/fpm/test/cli_test/cli_test.f90

Lines 168 to 171 in af6fa0d

    
           do j=1,command_argument_count() 
        
              call get_command_argument(number=j,value=big_argument,length=ilen) 
        
              write(*,'(*(g0))')i,'[',big_argument(:ilen),']' 
        
           enddo

The only drawback is that the CLI will fail consistently for all OS, which might look like a step back on first sight. The problem is that the namelist is reading single quotes over double quotes, and therefore mismatches the simple equals test at

fpm/fpm/test/cli_test/cli_test.f90

Line 126 in af6fa0d

call test_test('ARGS',act_args.eq.args)

with the input

-'arg1' -x 'and a long one'
+"arg1" -x "and a long one"

I tried to fix it by changing the delim argument for the namelist IO, but this doesn't help.

LKedward

Thanks again @urbanjost. This looks good. +1 to merge so that #188 can move forward with review.
Please open an issue to document the windows command line problems when you get a chance.

arjenmarkus · 2020-09-25T10:52:47Z

I was intrigued by this and therefore (a bit of procrastination ;)) I concocted a small test program and ran that on the following platforms: - Linux (gfortran and Intel Fortran) - plain Windows (Intel Fortran) - Cygwin (gfortran) - MinGW (gfortran) I used the following commands: cmdline.exe A B C D cmdline.exe 'A B C D' cmdline.exe "A B C D" cmdline.exe '*.f90' cmdline.exe "*.f90" The results are, well, interesting. The details are contained in the attached zip-file (tstcmdline is a simple shell script, rename it to testcmdline.bat for Windows - gmail did not accept in the zip-file). The short message: Linux and Cygwin essentially work in the same way, MinGW expands the file mask in the fourth command and Windows and MinGw do not group on apostrophes, only on quotation marks. Op vr 25 sep. 2020 om 11:43 schreef Laurence Kedward < notifications@github.com>:

…

***@***.**** approved this pull request. Thanks again @urbanjost <https://github.com/urbanjost>. This looks good. +1 to merge so that #188 <#188> can move forward with review. Please open an issue to document the windows command line problems when you get a chance. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#182 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN6YR6AS7SPKQMOTYIPQFDSHRQ3VANCNFSM4RT2CWCQ> .

everythingfunctional · 2020-09-25T13:30:52Z

@awvwgk , if I understand correctly, the namelist read (or maybe write) does not preserve quotes in a string? So in order to preserve a string round-trip, one must use single quotes (')?

@awvwgk and @arjenmarkus, this behavior matches what I believed was happening, Windows cmd environment (and I think batch behaves the same way) does not respect single quotes. Therefore, the CLI was properly parsing what Windows cmd had provided to it as the arguments, it just wasn't matching what the test expected.

Does this effectively lead us to contradicting requirements? The test requires the use of single quotes to preserve the arguments round trip through namelist, but Windows requires the use of double quotes to preserve the string as being a single argument. Is there a workaround we'd like to try and implement before merging, or should we go ahead and merge and just leave this as a known issue?

Thanks @awvwgk and @arjenmarkus for your help fleshing this out.

awvwgk · 2020-09-25T14:21:16Z

@awvwgk , if I understand correctly, the namelist read (or maybe write) does not preserve quotes in a string?

The short, but incorrect, answer is yes. The correct answer is that the namelist write step does not know about the type of quotation due to the recursive execution of the test binary and therefore cannot preserve anything.

So in order to preserve a string round-trip, one must use single quotes (')?

No, this is actually the wrong conclusion. The assumption that we can use single quotes (') in a cross platform application is wrong. Also the assumption that we can compare a command line by equality in the testsuite is not correct.

@awvwgk and @arjenmarkus, this behavior matches what I believed was happening, Windows cmd environment (and I think batch behaves the same way) does not respect single quotes. Therefore, the CLI was properly parsing what Windows cmd had provided to it as the arguments, it just wasn't matching what the test expected.

That is indeed correct. Single quotes are officially not supported to escape arguments in PowerShell and CMD on Windows. Note that testing cygwin on Windows introduces a POSIX shell, which will mitigate the problem, therefore cygwin is consistent with OSX and Ubuntu in the testsuite.

Does this effectively lead us to contradicting requirements?

No, the assumption for writing the test suite is assuming a POSIX shell and relying on an implementation detail of the namelist IO with GCC (and possibly other compilers as well) to always use single quotation marks.

Is there a workaround we'd like to try and implement before merging, or should we go ahead and merge and just leave this as a known issue?

My opinion on this is that the implementation is not portable and should be fixed before being merged. I gave it a try already, but I cannot find a simple way to fix the quotation mark issue since this information is just lost in the shell execution and the subsequent usage of namelist IO.

everythingfunctional · 2020-09-25T16:27:02Z

Just a point of clarification, which part of "the implementation is not portable"? If it is just the tests that are not portable, I think merging now and finding a different testing method later is acceptable. The other tests are sufficiently demonstrating the CLI to be working properly (or at least sufficiently for current requirements), and even the non-portable tests are demonstrating that it works under environments that preserve the intended behavior of the tests.

In my opinion, I don't think the CLI should be required to recombine "erroneously split" arguments. I think trying require a CLI to "fix" "improperly" split command line arguments in Windows environments would be akin to trying to ask it to "fix" "improperly" expanded glob (i.e. *.f90) arguments from a posix environment. It's just out of scope. It seems to me the current implementation is behaving as expected.

Unless I missed something.

awvwgk · 2020-09-25T17:18:46Z

Yeah, the test suite is not portable.

The PR is already lengthy and we can probably fix this later, of course.

fpm/test/cli_test/cli_test.f90

awvwgk · 2020-09-25T17:21:29Z

fpm/src/fpm_command_line.f90

+        version_text = [character(len=80) :: &
+           &  'VERSION:     0.0.0, Pre-alpha',                              &


The version specified in the fpm.toml is 0.1.0

Suggested change

version_text = [character(len=80) :: &

& 'VERSION: 0.0.0, Pre-alpha', &

version_text = [character(len=80) :: &

& 'VERSION: 0.1.0, Pre-alpha', &

urbanjost · 2020-09-25T19:31:26Z

I have a change that I think satisfies all the suggest changes and makes a test case I believe will work on MSWindows and POSIX platforms for all but edge conditions, with a change that should allow handling the edge conditions in the future. The -- ARGS option is intriguingly difficult on and MSWindows box. I have a test case I am working on for the case where the arguments are all the printable ASCII characters and the results are "interesting" with % characters in particular on windows. Since in this case the shell should have expanded most of the problem characters when the fpm command itself was called I find it difficult to find any kind of typical usage that this version fails on so far. I tried a simple rebase to avoid clobbering things again and apparently did not get the syntax right so I am hesitant to push this and it seems like a concensus is near. Should I just let this one lay aside for a moment and present this in #188 or I need help on how to make sure another push does not good changes out I am afraid. Suggestions?

pass settings extended help for each subcommand change commit= to ver= in fpm.toml ver= does not work either no specific version as ver= does not work for M_CLI2 add test program for CLI fix fpm.toml version reference remove --usage references from help text comment and clarify CLI unit test basic RUN subcommand restore fpm_command_line.f90 changes remove non-zero STOP for no parameters for testing spelling error in help use basename to make sure name is a simple name remove dash from executable name to see if it clears MSWindows build error try one more like previous build to clear error one more time like previous version to see if build error clears on MSWindows debug run to see PC variables make quoting of -- ARGS values less platform dependent and change test accordingly change .gitignore

everythingfunctional · 2020-09-25T20:22:22Z

So long as you made the commit on this branch, you should be able to run the commands

git pull --rebase
git push

and it should work fine. If not I should be able to recover anything lost and get it sorted out.

urbanjost · 2020-09-25T21:07:42Z

Well that was interesting. It did not work that simply as I had followed some web pages earlier trying to get past some of the earlier issues that required me to try to try something more involved; and I was following some git directions and got some interesting messages and then my machine crashed and now all but the ubuntu test have been running a long time without completing. I am not going to try anything else.

everythingfunctional · 2020-09-25T21:21:31Z

Ok, that looks like it clobbered, pretty much everything. I'm going to force push back to the latest state I had. Any chance you could redo the the changes you made?

urbanjost · 2020-09-25T22:40:03Z

Managed to break git(1). restoring from system backup. The web page seems to have a page from a previous file in the NEW PR that was never in the CLI branch.

urbanjost · 2020-09-26T03:12:03Z

The restore is complete, but need to take you up on that offer to reinstall you changes. I have no intention of changing it again unless there is something I do not see that did not restore.

urbanjost · 2020-09-27T18:32:35Z

I restored everything. One more time?

…

On 09/25/2020 5:21 PM Brad Richardson ***@***.***> wrote: Ok, that looks like it clobbered, pretty much everything. I'm going to force push back to the latest state I had. Any chance you could redo the the changes you made? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub #182 (comment) , or unsubscribe https://github.com/notifications/unsubscribe-auth/AHDWN3J5T44KP2XROUI3TDLSHUCWRANCNFSM4RT2CWCQ .

urbanjost · 2020-09-27T19:27:53Z

It makes it clear to me that there are disadvantages to Fortran only supporting the equivalent of system(3c) and not the exec(3c) familiy of C routines. Being required to call the system and thus exposing any spawned command to the vagaries of the OS shell and subsequent expansion and globbing makes it much harder to write a reliably portable program using subprocesses; especially because of the number of variations in MSWindows where escaping with ^ is not always reliable, and sometimes \ is used and that the program might be running in several environments with different parsing rules. Changing the M_CLI2 interface to " instead of ' makes this reasonably portable without having to determine the OS, but staying with a pure Fortran solution (ie. without calling C) the best solution seems to not requote the parameters as M_CLI2 does in this case, but to use the values directly obtained from the get_command_arguments(3f) function and conditionally requote them based on the system being called by execute_system_command(3f) for outliers with special characters which I will try to capture as a remaining issue, but specifically specifying the DELIM for the sake of the NAMELIST group tests and changing to a " from a ' seems to be doing a better job than many MSWindows commands and other packages. This experience helps justify the full round-trip test as only being exposed to the system parsing exposes the problem, on the bright side.

…

On 09/25/20I20 9:31 AM Brad Richardson ***@***.***> wrote: @awvwgk https://github.com/awvwgk , if I understand correctly, the namelist read (or maybe write) does not preserve quotes in a string? So in order to preserve a string round-trip, one must use single quotes (')? @awvwgk https://github.com/awvwgk and @arjenmarkus https://github.com/arjenmarkus , this behavior matches what I believed was happening, Windows cmd environment (and I think batch behaves the same way) does not respect single quotes. Therefore, the CLI was properly parsing what Windows cmd had provided to it as the arguments, it just wasn't matching what the test expected. Does this effectively lead us to contradicting requirements? The test requires the use of single quotes to preserve the arguments round trip through namelist, but Windows requires the use of double quotes to preserve the string as being a single argument. Is there a workaround we'd like to try and implement before merging, or should we go ahead and merge and just leave this as a known issue? Thanks @awvwgk https://github.com/awvwgk and @arjenmarkus https://github.com/arjenmarkus for your help fleshing this out. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub #182 (comment) , or unsubscribe https://github.com/notifications/unsubscribe-auth/AHDWN3L5FB6GELSDPICF4H3SHSLR3ANCNFSM4RT2CWCQ .

everythingfunctional · 2020-09-27T20:32:37Z

Despite the git related hiccups, this is now working (:smile:), so I'm going to go ahead and merge. Thanks for all your effort on this @urbanjost

urbanjost · 2020-09-27T21:52:17Z

Thank you! Thanks everyone for all the support an suggestions

…

On 09/27/2020 4:32 PM Brad Richardson ***@***.***> wrote: Despite the git related hiccups, this is now working (😄), so I'm going to go ahead and merge. Thanks for all your effort on this @urbanjost https://github.com/urbanjost — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub #182 (comment) , or unsubscribe https://github.com/notifications/unsubscribe-auth/AHDWN3JG5ZMOEEQGA72736TSH6OPFANCNFSM4RT2CWCQ .

certik requested review from LKedward and everythingfunctional September 21, 2020 15:15

LKedward reviewed Sep 22, 2020

View reviewed changes

fpm/fpm.toml Outdated Show resolved Hide resolved

LKedward previously approved these changes Sep 22, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

LKedward reviewed Sep 24, 2020

View reviewed changes

everythingfunctional approved these changes Sep 25, 2020

View reviewed changes

LKedward approved these changes Sep 25, 2020

View reviewed changes

awvwgk reviewed Sep 25, 2020

View reviewed changes

urbanjost changed the title ~~Pull request for CLI~~ CLI interface to further development of subcommands Sep 25, 2020

RESTORE

ea1dc19

RESTORE FROM BACKUP

a42775d

Fix test build issue hapenning on Windows

d653c11

everythingfunctional merged commit 90ddc6f into fortran-lang:master Sep 27, 2020

	& 'VERSION: 0.0.0, 20200920', &
	& 'VERSION: 0.0.0, Pre-alpha', &

		version_text = [character(len=80) :: &
		& 'VERSION: 0.0.0, Pre-alpha', &

CLI interface to further development of subcommands #182

CLI interface to further development of subcommands #182

Uh oh!

Conversation

urbanjost commented Sep 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!