Skip to content
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

Please revert commit 81eca07b348e55d391f357c137adaa8fd8f11dc0, it breaks cross compiled dokan fuse builds. #703

Closed
Daniel-Abrecht opened this issue Jun 23, 2018 · 9 comments
Labels

Comments

@Daniel-Abrecht
Copy link
Contributor

The commit 81eca07 changes all occurrences of #include <windows.h> to #include <Windows.h>, claiming it to correct a typo. Windows is case insensitive, and the capitalization of windows.h varies greatly across different windows SDKs, so this change neither fixes nor improves anything on windows.

Because linux is case sensitive and all linux cross compilers use lowercase for windows.h, the change broke cross compiling of dokan fuse and of applications which use dokan fuse.

Please revert commit 81eca07.

@Rondom
Copy link
Contributor

Rondom commented Jun 23, 2018

I and commit 0e8366c agree :-D

@kyanha
Copy link
Contributor

kyanha commented Jun 23, 2018

Does dokan_fuse/include/fuse_win.h need the same treatment?

@Daniel-Abrecht
Copy link
Contributor Author

Daniel-Abrecht commented Jun 23, 2018

All of the following files need the fix:

dokan_fuse/src/dokanfuse.cpp
dokan/dokan.h
dokan/dokani.h
dokan/list.h
dokan_fuse/src/fusemain.cpp
dokan_fuse/src/utils.cpp
dokan_fuse/src/fuse_helpers.c

dokan_fuse/include/fuse_win.h is not affected, because the include in there is only included if _MSC_VER is defined.

Commit 0e8366c only fixes dokan/dokan.h, which isn't sufficient. I've attached some build logs to show that every file I listed above indeed needs the fix: log.txt

Edit: I just noticed commit 0e8366c was from 2016 anyway.

@Rondom
Copy link
Contributor

Rondom commented Jun 24, 2018

Edit: I just noticed commit 0e8366c was from 2016 anyway.

That was just a tongue-in-cheek way of saying that I fixed this before ;-)

@Liryna
Copy link
Member

Liryna commented Jun 24, 2018

My bad 😞 made the change after trying to fix some sonarqube report and missed this point.
Thank you for the report @Daniel-Abrecht !

@Liryna Liryna closed this as completed Jun 24, 2018
@Liryna Liryna added the Bug label Jun 24, 2018
@kyanha
Copy link
Contributor

kyanha commented Jun 24, 2018

Hey @Liryna, is there a public SonarQube instance or SonarCloud for the dokan-dev projects? I'm pretty sure that SonarCloud is free for open source projects.

@Liryna
Copy link
Member

Liryna commented Jun 24, 2018

Hey @kyanha , Sonar allows opensource to use it for free. You can find the link in the readme: https://sonarcloud.io/dashboard?id=dokany

@kyanha
Copy link
Contributor

kyanha commented Jun 25, 2018

Cool, thanks! Do you have any plans to load the dokan-dotnet, dokan-sshfs, dokan-delphi, dokan-java, and/or dokan-ruby projects into it?

@Liryna
Copy link
Member

Liryna commented Jun 26, 2018

I have created a ticket to keep in mind for dokan-dotnet dokan-dev/dokan-dotnet#191
For other projects, have to see with other maintainers if they plan to do it (also probably add a CI like appveyor at first 😃 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants