-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Add vs2008_runtime #4728
Add vs2008_runtime #4728
Conversation
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
4c25ee8
to
186064c
Compare
186064c
to
7a9008e
Compare
recipes/vs2008_runtime/bld.bat
Outdated
@@ -0,0 +1,26 @@ | |||
for /F "usebackq tokens=3*" %%A in (`REG QUERY "HKEY_LOCAL_MACHINE\Software\Microsoft\DevDiv\VC\Servicing\9.0\IDE" /v UpdateVersion`) do ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know how to query the runtime library version number from the registry or CMake for VS 2008, @isuruf? I'm pretty sure what I have here is wrong. Though without a Windows machine at hand, it's proving difficult to find out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed an untested fix. Let's see if it works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @isuruf. Nice fix!
Now if we can only figure out why Edit: Maybe it's not included with this install? 😕 Edit 2: Raised as issue ( appveyor/ci#2008 ). |
db967ea
to
5ef73fc
Compare
Thanks for exploring this with me @isuruf. I'm wondering if AppVeyor actually installs |
901c50e
to
0b6b0e1
Compare
These should hopefully have the missing `vcomp90` library in them. So add this hack to `bld.bat` temporarily to see if this fixes the issue.
0b6b0e1
to
9c06135
Compare
@@ -0,0 +1,33 @@ | |||
package: | |||
name: vs2008_runtime | |||
version: "9.0.30729.6161" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakirkham, I updated to 9.0.30729.6161 which is the SP1 MFC security update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds fine to me. As long as we are equal to or greater than (which this is) defaults
copy, I think we should be ok.
recipes/vs2008_runtime/bld.bat
Outdated
set "ARCH_DIR=amd64" | ||
) | ||
|
||
cd C:\Windows\WinSxS\%ARCH_DIR%_microsoft.vc90.openmp_*_%PKG_VERSION%_* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using the exact version here, so all of the dlls are actually coming from the SP1 MFC update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add the original link to where these were downloaded from in a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done above.
"vcomp90" | ||
] %} | ||
{% for each_lib in libs %} | ||
- if not exist "%PREFIX%\\{{ each_lib }}.dll" exit 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defaults is placing the dlls in %PREFIX%
and %LIBRARY_BIN%
, so did the same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out.
[ci skip]
extra: | ||
recipe-maintainers: | ||
- gillins | ||
- isuruf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went ahead and added you as a maintainer, @isuruf. Please let me know if that is ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
|
||
extra: | ||
recipe-maintainers: | ||
- gillins |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gillins, are you still happy to be a maintainer here?
recipe-maintainers: | ||
- gillins | ||
- isuruf | ||
- mingwandroid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mingwandroid, are you still happy to be a maintainer here?
- gillins | ||
- isuruf | ||
- mingwandroid | ||
- msarahan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msarahan, are you still happy to be a maintainer here?
So do we want to keep the vcredist installs in this recipe and make a note that it requires administrative privileges or would we rather add them to |
We can pass through the env variable |
That seems like a reasonable suggestion. |
One last question, could we validate these downloads against some kind of checksum? |
Done |
Beautiful. Thanks @isuruf. :) I think we are ok to merge. |
@conda-forge/core @conda-forge/staged-recipes, this is ready for a final review. :) |
|
||
about: | ||
home: http://www.microsoft.com | ||
license: Proprietary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. They tell us that we can redistribute the runtime libraries. Anything not in that list comes from this Microsoft Redistribution, which exists for this purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also from a very old discussion that happened on this same topic would highlight this comment as well.
Anything else? |
Is it alright if we merge? |
Great work guys |
Thanks everyone! |
Adds a package for
vs2008_runtime
forconda-forge
using AppVeyor. Uses the strategy proposed in this comment due to some oddities with AppVeyor's VS 2008 install.