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

Fix Missing api-ms-win-core-path-l1-1-0.dll #224 #237

Merged
merged 1 commit into from Mar 23, 2016

Conversation

Projects
None yet
5 participants
@takuya-takeuchi
Copy link
Contributor

takuya-takeuchi commented Mar 15, 2016

Hi guys!!
I want to merge my source code to fix #224.
I believe that you have already checked my source code on issue board.
Just in case, please check once again.

I checked my source code by build it on Windows 7 and running on Windows 7, 8.1 and 10.

Check method is the following.
https://github.com/Microsoft/CNTK/wiki/Setup-CNTK-on-Windows#trying-cntk-with-cpu

Test, train and write is completed.

NOTE
On Windows 8 which have no Visual Studio, OS requires Visual C++ Redistributable for Visual Studio 2012. Is it correct? I thought CNTK requires only VS2013 C++.

I never pull request.
So if you have any problem, let me know.

Thank you!!

@msftclas

This comment has been minimized.

Copy link

msftclas commented Mar 15, 2016

Hi @takuya-takeuchi, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, MSBOT;

@alexeyo26

This comment has been minimized.

Copy link
Member

alexeyo26 commented Mar 15, 2016

Takuya, thank you. But this needs a little revision. PathRemoveFileSpec is depreciated and for the good reason: it could cause a buffer overrun. At the same time PathCchRemoveFileSpec, its replacement relies on the dll that was introduced in Win 8.

So, I believe the fix should check OS version and in case its "6.2." or greater, then PathCchRemoveFileSpec should be use. Otherwise PathRemoveFileSpec for older versions. See https://msdn.microsoft.com/en-us/library/windows/desktop/ms724832(v=vs.85).aspx and https://msdn.microsoft.com/en-us/library/windows/desktop/ms724429(v=vs.85).aspx on detecting OS Version

Thank you,
Alexey

@alexeyo26

This comment has been minimized.

Copy link
Member

alexeyo26 commented Mar 15, 2016

Takuya, one more thing. You wrote

On Windows 8 which have no Visual Studio, OS requires Visual C++ Redistributable for Visual Studio 2012. Is it correct? I thought CNTK requires only VS2013 C++.

Are you sure you were compiling on VS 2013 and (even more important) you don't have a mix of VS2012 and VS2013 on your system?

@takuya-takeuchi

This comment has been minimized.

Copy link
Contributor

takuya-takeuchi commented Mar 15, 2016

I'm sorry to trouble you.
To brush up source code, please give me your hand.

Are you sure you were compiling on VS 2013 and (even more important) you don't have a mix of VS2012 and VS2013 on your system?

Oh really? I did not know. I never see this condition in wiki.
My system is mix of 2008, 2010, 2012, 2013 and 2015.
OK, but i can prepare system which include only 2013.

Thank you.

@alexeyo26

This comment has been minimized.

Copy link
Member

alexeyo26 commented Mar 16, 2016

Takuya, terribly sorry, This VC++ 2012 dependency has nothing to do with your system and being generated by ACML. I will update setup instructions right away.

Anyway we do NOT recommend of having a mixture of different Visual Studio installations on one machine (don't mix with different VC++ runtime, which is perfectly fine). We mention it here https://github.com/Microsoft/CNTK/wiki/Setup-CNTK-on-Windows#building-cntk

Thank you,
Alexey

@takuya-takeuchi

This comment has been minimized.

Copy link
Contributor

takuya-takeuchi commented Mar 16, 2016

Hi Alexey. Thank you for your kindly support.

So, I believe the fix should check OS version and in case its "6.2." or greater, then PathCchRemoveFileSpec should be use. Otherwise PathRemoveFileSpec for older versions.

I tried the following source code on Windows 7.

    if (IsWindows8OrGreater())
    {
        auto hr = PathCchRemoveFileSpec(&path[0], path.size());
        if (hr == S_OK) // done
            path.resize(wcslen(&path[0]));
        else if (hr == S_FALSE) // nothing to remove: use .
            path = L".";
    }
    else
    {
        auto hr = PathRemoveFileSpec(&path[0]);
        if (hr != 0) // done
            path.resize(wcslen(&path[0]));
        else
            path = L".";
    }

But this code occur application crash.
Windows7 does not have api-ms-win-core-path-l1-1-0.dll.
So even if developer was successful to build CNTK on Windows 7, application will crash.

So we cannot use the above code and resolution.

I am not familiar with C++. I guess I may resolve use LoadLibrary to call 2 function based on OS version. Is it ok?
Do you have any idea?

@alexeyo26

This comment has been minimized.

Copy link
Member

alexeyo26 commented Mar 16, 2016

Takuya, then try a different approach based upon dynamic linking. I.e. try to load the dll in question and base the logic on whether you can load it like shown in this example https://msdn.microsoft.com/en-us/library/windows/desktop/ms686944(v=vs.85).aspx

@alexeyo26

This comment has been minimized.

Copy link
Member

alexeyo26 commented Mar 23, 2016

Takuya, thank you a lot! In fact BOTH of your fixes work. The reason the 1st one crashed is the necessity to add api-ms-win-core-path-l1-1-0.dll in "Delay Loaded Dlls" in CNTK project properties (Linker Section).

Your code generated a couple of ideas about a couple of other things we need to fix. We're implementing it now and will tell, when it's there.

And one more thing. When you're working on a Pull Request and make a new Commit, please place a new comment about it in "Conversation" (i.e. here). The reason is GitHub does not notify about new commits to Pull Request, that's why I spotted your new contribution just today and not on Saturday when you have actually made it.

Thank you one more time. We'll tell you about the progress.
Alexey

@takuya-takeuchi

This comment has been minimized.

Copy link
Contributor

takuya-takeuchi commented Mar 23, 2016

You are welcome.

A few weeks, I have been excited that I may be able to contribute for open source community.
At same time, It was anxiety. My modification may break something? I may take other developer's time to support to me...

But that was a very good experience.

Thank you, too!!

@alexeyo26

This comment has been minimized.

Copy link
Member

alexeyo26 commented Mar 23, 2016

Takuya, after internal consideration we decided we want to implement _your_ fix, but during these days Master changed in such a way, that our system can't build your pull request (it's based on the version of Master, that is too old). Please, update the master in your fork and submit a new commit - we'll proceed with it ASAP.

Use the code, that is in your second commit (with LoadLibrary())

Thank you,
Alexey

@takuya-takeuchi takuya-takeuchi force-pushed the takuya-takeuchi:#224 branch from 208c1ad to c51b913 Mar 23, 2016

@takuya-takeuchi

This comment has been minimized.

Copy link
Contributor

takuya-takeuchi commented Mar 23, 2016

OK, I retrieved latest source and pushed again.

@takuya-takeuchi

This comment has been minimized.

Copy link
Contributor

takuya-takeuchi commented Mar 23, 2016

Use the code, that is in your second commit (with LoadLibrary())

Oh, I'm very sorry. I missed the following direction... what should I do?

@alexeyo26

This comment has been minimized.

Copy link
Member

alexeyo26 commented Mar 23, 2016

You did everything right. Don't worry, We're working with your pull request now...

@alexeyo26

This comment has been minimized.

Copy link
Member

alexeyo26 commented Mar 23, 2016

Attention MS Reviewer: Build 3484

@wolfma61

This comment has been minimized.

Copy link
Contributor

wolfma61 commented Mar 23, 2016

👍

@svccntk svccntk merged commit c51b913 into Microsoft:master Mar 23, 2016

@alexeyo26

This comment has been minimized.

Copy link
Member

alexeyo26 commented Mar 24, 2016

Attention MS Reviewer: Build 3485

@alexeyo26

This comment has been minimized.

Copy link
Member

alexeyo26 commented Mar 24, 2016

Takuya, thank you a lot for the contribution - it's in Master now. Enjoy CNTK!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment