Notepad++ File Status Auto-Detection not working

chcg

@Claudia-Frank The builds at appveyor not just contain the trunk checkins, but also pull requests which might contain something totally different. Sorry, I meant predecessor.

I created a branch from the 7.5.1 tag and modified the appveyor config to build VS2013 vs. VS2015 on the appveyor image 2015. So this should reveal if already the switch to VS2015 is causing the trouble.

See https://ci.appveyor.com/project/chcg/notepad-plus-plus/build/1.0.114, build from https://github.com/chcg/notepad-plus-plus/commits/winxp_autodectection_investigation2

Since the checkin of the json parser(https://github.com/notepad-plus-plus/notepad-plus-plus/commit/b033d907b29f42f570c21b0bfc0ac60c257f08c4) it is no longer possible to build with VS2013 due to the lack of c++11/14 features needed.

Claudia Frank

@chcg

Thx for clarification - I will give it a try later and let you know.

Cheers
Claudia

Claudia Frank

@chcg

binary with PlatformToolset=v120_xp works
but binary with …=v140_xp does not.
Hmmm, I see the binaries are different in size
but I assume you just changed the toolset version
and the rest was kept the same, wasn’t it.

I already searched the web, and there are some reports that there are problems
on windows xp when using VS2015 onwards but it looks like all of them were solved
by setting the compatibility flag aka toolset. Strange.

Cheers
Claudia

Vitaliy Dovgan

To be fully compatible with Windows XP, the following compiler’s option may need to be specified:

/Zc:threadSafeInit-

This one relates to the “magic static” feature in C++ that depends on new TLS implementation available only from Windows Vista/7.
In case of Notepad++ plugin (i.e. a DLL file) the absence of this compiler’s option leads to crash (under Windows XP) while trying to use a static variable. In theory it should not have the same dramatic effect in an executable, but who knows? I think it’s worth trying to comile Notepad++ with this option specified.

Claudia Frank

@Vitaliy-Dovgan

thank you very much for the hint.

@chcg

If you wouldn’t mind to build one with this flag I would give it a try to see if it resolves the issue.

Cheers
Claudia

chcg

There were 3 changes in the appveyor config:

VM image, as Visual Studio 2013 doesn’t contain vs2015
calling Microsoft Visual Studio 14.0\VC\vcvarsall.bat for vs2015 build
using notepadPlus.vs2015.vcxproj for building the vs2015 version.
couldn’t be avoided. The sourcecode itself is identical and either it is something with the vcxproj for vs2015 which causes the issue or the vcvarsall.bat + the toolset.

Maybe the build of scintilla could also have an influence.

I will do a rebuild with just the toolset changed and using the notepadPlus.vcxproj also for VS2015.

And another one with /Zc:threadSafeInit-

chcg

First test is available here:
https://ci.appveyor.com/project/chcg/notepad-plus-plus/build/1.0.117

VS2013 config with PlatformToolset=v140_xp

Claudia Frank

@chcg

v120 build works, v140 does not.

Cheers
Claudia

chcg

VS2015 build with /Zc:threadSafeInit- at https://ci.appveyor.com/project/chcg/notepad-plus-plus/build/1.0.118

Claudia Frank

@chcg

thx, will do the test and report the status later (2-3 hours).

Cheers
Claudia

Claudia Frank

@chcg

sorry but additional compiler flag does not work either.

Cheers
Claudia

Vitaliy Dovgan

Looks like time to use additional logging then. I mean, logging around file status detection. (I briefly looked at the code, and it’s not obvious at the first glance what exactly can go wrong.)

Vitaliy Dovgan

I’ve found out the cause of the problem. In VS2015, the stupids from Microsoft have broken _wstat for Windows XP! According to a simple change inside the file “PowerEditor\src\ScitillaComponent\Buffer.cpp”:

int st = generic_stat(_fullPathName.c_str(), &buf);
char s[512];
sprintf(s, "Buffer::checkFileState() -> %d, errno %d, FileName \"%S\"", st, errno, _fullPathName.c_str());
writeLog(_T("1.txt"), s);

one can notice that _wstat always returns -1 under Windows XP with errno 22 (EINVAL).
So the fix seems to be to replace _wstat with some native WinAPI function, since we can not rely on VS2015’s CRT anymore.

chcg

See https://github.com/joncampbell123/dosbox-x/issues/403 and https://github.com/joncampbell123/dosbox-x/issues/438 which also suffers from this issue. From https://stackoverflow.com/questions/6566978/whats-the-difference-between-struct-stat64-and-struct-stati64-on-win32, maybe we are facing a 64bit issue, see https://msdn.microsoft.com/en-US/library/14h5k7ff.aspx. What happens on using _wstat64.

Vitaliy Dovgan

Yes, it could be that _USE_32BIT_TIME_T needs to be defined for Windows XP. On the other hand, it was reported that defining _USE_32BIT_TIME_T, in its turn, creates problems with std::chrono… Just madness.

Vitaliy Dovgan

Basically, _wstat is used there just to get the file modification time and check if it is read-only or not. There are 2 corresponding WinAPI functions that can be used instead, without being relying on _wstat at all: GetFileTime and GetFileAttributes. Using them, any further breaking change in _wstat will not affect Notepad++ at all!

Vitaliy Dovgan

As far as I understand, an honour to implement the proposed change is mine :) As Notepad++ is the base for all its plugins, I consider such task as quite important. I don’t promise I do it quickly, but I already have an idea what exactly to do. Since _wstat is already called through a macro, the most straightforward change will be to call own function instead of _wstat via the very same macro, providing own structure returned by the function, and the structure will be filled by the WinAPI functions. So the code will remain similar at the surface, but will not rely on _wstat anymore.

Vitaliy Dovgan

OK, guys, I want to make everything in the proper way :) Do you usually create a new git branch to use it for the pull request? Or just directly modify the “master” branch? And is there any possible hints/problems regarding authorization (such as special write rights, SSH, etc.)?

chcg

See https://github.com/notepad-plus-plus/notepad-plus-plus/pulls e.g.
https://github.com/notepad-plus-plus/notepad-plus-plus/pull/4172 made from branch cmeriaux:fix_shortcut_validity. So branching your forked master is the better way, so you could easily pull upstream master again for future work. See also https://github.com/notepad-plus-plus/notepad-plus-plus/blob/master/CONTRIBUTING.md. There shouldn’t be problems regarding auth against your github fork.

chcg

Maybe check this one https://gist.github.com/Chaser324/ce0505fbed06b947d962 regarding git usage with github.