arrayfire / arrayfire-rust

Rust wrapper for ArrayFire
BSD 3-Clause "New" or "Revised" License
810 stars 58 forks source link

[Build] Build scripts silently ignores non-accessible DLLs #308

Closed phil-opp closed 2 years ago

phil-opp commented 2 years ago

Description

When looking for available DLLs, the build scripts treats all access errors like files that don't exist:

https://github.com/arrayfire/arrayfire-rust/blob/7cfde6b827f2976767f4e5dc6b8913fe3ef0140e/build.rs#L71-L76

If the current user has no read permissions on the af.dll, it is not added to the backends list:

https://github.com/arrayfire/arrayfire-rust/blob/7cfde6b827f2976767f4e5dc6b8913fe3ef0140e/build.rs#L419-L424

As a result, no arrayfire library is passed to the linker, which then throws a lot of errors because for the various af_* functions (see below).

The arrayfire build script should really trigger an error or panic if no backends are found instead of letting the users run into linker errors. Ideally, the error message would also mention that some files were found, but were not be accessible.

Build Environment

Compiler version: rustc 1.57.0 (f1edd0429 2021-11-29) Operating system: Windows 10 Build environment: (described above)

Error Log

Lots of errors like these:

error LNK2019: unresolved external symbol af_retain_array referenced in function _ZN3std10sys_common9backtrace28__rust_begin_short_backtrace17h3e2e43e5f6399194E
9prady9 commented 2 years ago

If the current user has no read permissions on the af.dll, it is not added to the backends list:

I don't see a problem in this scenario. If the current user doesn't have permissions, that should be fixed outside of rust's context. If a file can't be read by an user, it is essentially a non-existent file to that user. There is no way to know what are it's permissions. At least, I am not aware of any such mechanism. If you are aware of something like that, please do share and I shall look into it. You are also welcome to raise a PR with the necessary fix if that is something you are interested in.

phil-opp commented 2 years ago

The main problem is that there is no error or warning if no backends are found at all, i.e. when the backends list is empty. There is no way the linking can succeed in this case, so I think it makes sense to treat this case as an error in my opinion.

It's true that non-readable files cannot be used and must be fixed manually. However, if the user explicitly points the AF_PATH to such a directory, I don't think that silently ignoring these access errors is a good idea, as it gives the impression that the AF_PATH is valid and working.

So my proposal is: If no backends are found, we throw an error from the build script (it would result in an linker error later anyway). We also keep track of the I/O error that occurs for the main af.dll/af.so and display it as an additional help message. For example:

Error: No arrayfire backends found

help: - searched paths: ...
      - AF_PATH set to ....
      - a file named `af.dll` exist at ..., but an error occured while accessing it: ...
9prady9 commented 2 years ago

The main problem is that there is no error or warning if no backends are found at all, i.e. when the backends list is empty. There is no way the linking can succeed in this case, so I think it makes sense to treat this case as an error in my opinion.

I wonder when such a scenario would happen unless it is a broken installation or broken build 🤔 Nevertheless, I agree an early break while build script is running might help parsing a larger linker error message. Feel free to raise a PR for the same if you can, Otherwise I shall look into it as soon as I can.

It's true that non-readable files cannot be used and must be fixed manually. However, if the user explicitly points the AF_PATH to such a directory, I don't think that silently ignoring these access errors is a good idea, as it gives the impression that the AF_PATH is valid and working. ... a file named af.dll exist at ..., but an error occured while accessing it: ...

How do you propose accessing the access of a backend(library file) ? Other than checking for it's existence, any other solution will end being similar to using the library file like the rust create does.

phil-opp commented 2 years ago

I wonder when such a scenario would happen unless it is a broken installation or broken build

It was a non-standard installation in our case that was indeed broken :smile:. However, debugging it was much more difficult because the build script succeeded without errors. I'll create a PR to add an error message for that case.

How do you propose accessing the access of a backend(library file) ?

We can check the error kind of the io::Error, which is NotFound if the file doesn't exist and PermissionDenied if the permissions are wrong. At least it was like that in our case.

phil-opp commented 2 years ago

I opened a PR at #310. I used a slightly different approach for reporting I/O errors, so please let me know what you think.

9prady9 commented 2 years ago

I opened a PR at #310. I used a slightly different approach for reporting I/O errors, so please let me know what you think.

Thank you for the contribution, I have just left couple of minor comments. Please have a look.