Skip to content

Conversation

@tmaklin
Copy link

@tmaklin tmaklin commented Apr 12, 2022

Hi,

Thanks for creating fastspar, very useful and easy to use!

I was having some trouble with writing/reading from files that are in a directory different from where fastspar was run because the directory paths either didn't exist or had typos in them. I added a bunch of simple checks to deal with this. These checks do the following:

  1. fastspar will now check if the output for --correlation or --covariance is in a directory (path contains a '/') and exit with an error message if the directory (path stripped of all characters after the last '/') doesn't exist or isn't accessible. Previously the program ran fine without errors but didn't produce any output.
  2. fastspar_bootstrap will check if --prefix is in a directory and exit if that directory doesn't exist. Previously the program produced a segmentation fault if this was the case.
  3. fastspar_pvalues checks performs the same check as 1. for --outfile, and the same check as 2. for --prefix. Previously errors in --outfile caused the program to run fine but produce output, and errors in --prefix resulted in a slightly misleading error message about not having the correct number files in the folder.

The checks for the directory use <dirent.h> which is not available on windows systems, so I wrapped the relevant code in a preprocessor directive which removes it when compiling on windows.

@scwatts
Copy link
Owner

scwatts commented May 4, 2024

Thanks for spending the time putting this together. What do you think about removing the Win32 preprocessor guard and dropping any implicit/supposed Windows support entirely?

@tmaklin
Copy link
Author

tmaklin commented Jun 4, 2024

Hi, think I originally added it just to be sure that it doesn't break anything, I have no strong feelings about supporting or not supporting windows here 😁

C++17 has a more portable way to do these checks under the <filesystem> header so that would be preferable for making the checks work on Windows.

@scwatts
Copy link
Owner

scwatts commented Jul 27, 2024

Let's avoid attempts to preserve any potential/supposed support for Windows since that was never an explicit goal - there is value in supporting this but those changes would fit better in a more comprehensive PR. I'm not aware of any native Windows users, and regardless there is the option of WSL2 to run FastSpar.

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.

2 participants