optimize file listing

[ghost]

This PR is an attempt to optimize the file listing subroutine as reported at Discourse [1].
This implementation is pure Fortran using the iso_c_binding module, but it depends on the preprocessor to compile. The trouble is that the C types struct dirent and struct stat have different sizes/contents in different platforms. [2-5]

This PR was tested on Windows with MinGW 32 bit and MinGW 64 bit. On linux, it was tested with Debian 32 bit and Debian 64 bit. In all other platforms, this PR has no effect.

To enable this feature, build fpm with

fpm build --flag "-DMINGW64"
fpm build --flag "-DMINGW32"
fpm build --flag "-DLINUX64"
fpm build --flag "-DLINUX32"

CC @zoziha, @LKedward

[1] https://fortran-lang.discourse.group/t/fpm-may-be-io-blocking-problem-feedback-in-fpm-test
[2] https://github.com/mirror/mingw-w64/blob/master/mingw-w64-headers/crt/dirent.h
[3] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/dirent.h.html
[4] https://github.com/mirror/mingw-w64/blob/master/mingw-w64-headers/crt/sys/stat.h
[5] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_stat.h.html

[zoziha]

Give an example to test it.
Computer configuration: ThinkPad S2 3rd (win-10, i5-8250U)
Description: time <command>
Comand: fpm test help-list
Example1: ./help-list

Command being timed: "./help-test"

real    0m2.075s
user    0m0.000s
sys     0m0.047s

Example2: fpm(brocolis branch fpm build --profile release --flag "-DMINGW64") test help-list

Command being timed: "fpm test help-list"

real    0m2.851s
user    0m0.015s
sys     0m0.000s

Example3: fpm(0.3.0 release fpm build --profile release) test help-list

Command being timed: "fpm test help-list"

real    0m7.762s
user    0m0.015s
sys     0m0.031s

I have not tested the more specific situation. But this example mainly shows that the time it takes to run the executable program help-test.exe that has been generated using the fpm test command has not changed.
I don’t know enough about the code implementation of fpm, but I feel that this PR does not improve the time of the most time-consuming query file information part of fpm test on windows-gfortran?

---

I'm sorry, I used gdb to perform partial testing of the fpm code, and found that I incorrectly had used --flag "-DMINGW64" to compile the master branch of brocolis's forked branch, so the result is consistent with the fpm 0.3.0 release version, I'm very sorry ,This is my mistake!
And I corrected the above results, it is very thankful, thank you @brocolis @LKedward very much, the running time of fpm test is much less.

[LKedward]

Thanks Carlos @brocolis - I'll try this out later and look over the code. Thanks for testing it @zoziha - I'm surprised there's no improvement, there is perhaps another significant bottleneck somewhere.

[zoziha]

I'm very sorry ,I made a mistake!😥
I corrected the above results, it is very thankful, thank you @brocolis @LKedward very much❤, the running time of fpm test is much less.👍

[LKedward]

No worries @zoziha and thanks for checking it again - glad to see that it's an improvement (great work @brocolis!).

The trouble is that the C types struct dirent and struct stat have different sizes/contents in different platforms.

Yes this is true. We could solve it by wrapping the relevant code in a c function but this would prevent us from creating single-file bootstrap versions which are really useful. This may not be a big problem since we only officially support building fpm with gfortran (#205), so I don't mind the implementation here if we can be sure it won't cause problems on the most common platforms, but I'm not sure how to guarantee that. It's nice that the feature can be enabled/disabled for the time being.

[ghost]

Hi @zoziha, @LKedward, thanks for the feedback!.

Yes, the types in this PR are basically the result of the C preprocessor running on my machine, translated to Fortran syntax; I don't know if the code works on other linux distros. A C wrapper would be more maintainable and reliable, I agree. Should we go in that direction? It's unfortunate that we'd lose the single-fortran-file bootstrap version.

[awvwgk]

Should we go in that direction? It's unfortunate that we'd lose the single-fortran-file bootstrap version.

As long as we have a single-file Fortran version that can be used in a two step bootstrapping process we would be fine. But I agree that retaining the single-file option for an one step bootstrapping process has some value.

[ghost]

As long as we have a single-file Fortran version that can be used in a two step bootstrapping process we would be fine. But I agree that retaining the single-file option for an one step bootstrapping process has some value.

Thanks @awvwgk, I'll try a version with a tiny C wrapper.

[LKedward]

I'll try a version with a tiny C wrapper.

Thanks @brocolis - I think this is the right way to go since there are other file system functions that will need rewriting in a similar manner. Perhaps you can use preprocessor directives to enable the existing non c implementation just for when we produce the single-file version as Sebastian mentioned.

[ghost]

Added C wrapper. To enable this feature, build fpm with --flag "-DENABLE_C_WRAPPER".

[LKedward]

Great stuff @brocolis! However I think we should use the C wrapper by default and only require a compiler flag to enable the alternate Fortran code for when we want to put together a single-file version without the C code. What do you think?

[awvwgk]

How about using -DFPM_BOOTSTRAP to enable components not used for the regular production version?

[ghost]

CI Build (macos-latest) failed with

CMD= new --help EXITSTAT=0 CMDSTAT=0 MESSAGE= Program received signal SIGSEGV: Segmentation fault - invalid memory reference.

I don't have access to macOS machine, can't run it under debugger.. need help here.

[awvwgk]

I think we can find a more meaningful file name than c.c for the file system functionality.

[ghost]

I think we can find a more meaningful file name than c.c for the file system functionality.

I agree. Do you have any suggestion?

[ghost]

CI Build (macos-latest) failed …

I tried renaming the is_dir function, just to be sure the test was running the correct code, but it had no effect in macOS CI. I can't fix this issue.

[LKedward]

@everythingfunctional do you have a Mac machine and can possibly help out here?

[everythingfunctional]

@LKedward , I may have some time later today or tomorrow to give it a try. Are there any special instructions for how I should start? Or just build this branch and run the test suite?

[LKedward]

Thanks! The error appears to be in new-test which tests the fpm new command. Any help would be appreciated, even if you're able to tell us on which line(s) it occurs or give a full back-trace, that could be enough to point us in the right direction.

[everythingfunctional]

Ok, running the command fpm test --target new-test --runner gdb on my Mac, I get the following:

...
CMD= new --help EXITSTAT=0 CMDSTAT=0 MESSAGE=

Thread 2 received signal SIGSEGV, Segmentation fault.
0x00007fff20520588 in ?? ()
(gdb) bt
#0  0x00007fff20520588 in ?? ()
#1  0x00007ffeef4ccb30 in ?? ()
#2  0x0000000100020562 in f_string_cptr (__result=<error reading variable>, .__result=0xa004f7000, cptr=<error reading variable: Attempt to dereference a generic pointer.>) at ././src/fpm_strings.f90:146
Backtrace stopped: frame did not save the PC

In my experience gdb on Mac is very flaky, hence why it doesn't seem to have been able to provide a full backtrace and I wasn't really able to inspect the state of things much. This gives you a place to start looking, but not a lot to go on. If there are other things you'd like me to try I can, just let me know.

[ghost]

Thanks @everythingfunctional ! I will look into the issue.

[arjenmarkus]

I have no experience with MacOS but perhaps the Intel Inspector tool from Intel oneAPI can help? I have found it to be quite useful on Windows for memory-related issues. Op di 13 jul. 2021 om 06:03 schreef Carlos Une ***@***.***>:

…

Thanks @everythingfunctional <https://github.com/everythingfunctional> ! I will look into the issue. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#507 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN6YRZYQWCBB5MTCJB22LTTXO3JRANCNFSM47RL4OBA> .

[LKedward]

Hi Carlos @brocolis - I've implemented a small fix for OSX based on this conversation: rust-lang/libc#414.
It seems to be working, let me know what you think.

[everythingfunctional]

I would call it filesystem_utilities.c, but the rest looks ok. I like that we are able to switch back to a pure Fortran implementation for producing a single file "bootstrap" version.

[ghost]

Hi Carlos @brocolis - I've implemented a small fix for OSX based on this conversation: rust-lang/libc#414.
It seems to be working, let me know what you think.

@LKedward looks good. Thank you for the fix!.

[ghost]

I would call it filesystem_utilities.c, but the rest looks ok. I like that we are able to switch back to a pure Fortran implementation for producing a single file "bootstrap" version.

@everythingfunctional thank you for the suggestion!. I'll rename it.

[LKedward]

Great stuff Carlos @brocolis! With a few more reviews I think this is good to go 👍

[everythingfunctional]

LGTM Great work guys

[awvwgk]

Thanks a lot, this will be a significant improvement.

[LKedward]

Carlos @brocolis, are you able to resolve the merge conflicts in this, or do you need help?

[ghost]

@LKedward I'm not able to resolve the merge conflicts, I need help. 'Resolve conflicts' button appears disabled for me.

[LKedward]

No problem @brocolis - I can take a look at it.
(When the button is disabled, it just means you need to do the merge in your local copy instead of via the web editor.)

[LKedward]

Many thanks Carlos @brocolis and thanks everyone for reviewing! With three approvals, I'll now merge.