-
-
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
Vs2008 appveyor cmake #617
Conversation
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipes/vs2008_express_vc_python_patch:
|
In the past we have done BSD 3-Clause (e.g. |
version: 1.0 | ||
|
||
source: | ||
url: https://github.com/menpo/condaci/raw/master/vs2008_patch.zip |
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.
Should we pick a tag or commit maybe?
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.
v0.4.8 is the latest tag.
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.
Should add that personally I would also be ok with just including those files with the recipe. This seems like a special case recipe where that is totally appropriate.
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.
Part of the reason I say that, @pelson, is because @patricksnape plans to remove these from his repo. So, it might be best for both of us if they live somewhere new. Here seems like as good as any place.
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.
Should add there are other cases where we are making the same decision (e.g. the toolchain
, conda-forge-build-setup
, etc.)
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.
It isn't ideal but if necessary I'm not against it.
Does this whole package need to be put in conda-forge-build-setup? At least, I guess that should be the package which depends on this one?
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.
IMHO it would be nice to keep this separate if this is an option. That way someone has the option of running this locally if needed. Though I'm perfectly fine with making it a dependency of conda-forge-build-setup
and running it in the scripts there.
What do you think @msarahan and @patricksnape?
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.
As mentioned below, the copying into Program Files
would be problematic.
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.
I've been persuaded. Moving this to conda-forge-build-setup seems like the best way forward.
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.
Liking the separate package still for local use though.
So what is the deal, do we just install this when we install conda-build in AppVeyor and everything works? I assume the intention is not to make this an explicit dependency in the recipes? |
Yeah if you install this as a build-dep it will fix Appveyor - very smart idea @msarahan! |
We probably only want to run it on x64? |
So, there are a few ways I could see one going with this.
Ideally, I would want to go for 3 because there is one line that would muck with a user's system. In the worst case, I could get behind 2 if we automatically disable that one line somehow, but provide the option to override it. We should really avoid 1. If we are looking at that closely and 3 is not achievable, we should just go for 2. Though I'm really hoping 3 is an option. |
👍 - I have been wanting an excuse to implement a metapackage which is installed before we conda-build for a little while now, seems like now is the time to do that. We can make this package a runtime dependency of that metapackage on Windows. Controlling things like the conda-build version would also be in scope for that metapackage. |
@@ -0,0 +1,2 @@ | |||
call %PREFIX%\Scripts\setup_x64.bat | |||
copy "C:\Program Files (x86)\Microsoft Visual Studio 9.0\VC\bin\vcvars64.bat" "C:\Program Files (x86)\Microsoft Visual Studio 9.0\VC\bin\amd64\vcvarsamd64.bat" |
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.
I'd much prefer explicit to implicit given what this is doing. Could we please make this a proper script that one calls explicitly?
cc @JanSchulz |
Just for reference: IMO this should be part of a "If you want to have a proper build environment, you need a working VS install, which includes this fix. You can achieve that fix by installing the vs2008_express_vc_python_patch package with admin rights." wiki page detailing how to setup conda locally or on a CI service. IMO adding this thing to a script (aka "...and you then have to run ... as admin") would also make it failsaver as some checks could be added: is VS installed,... On my system, I don't have
I do have:
|
Totally agree @JanSchulz. This came up in a question awhile back so I put it on the list. I think we have all nixed it. I think we have all since agreed that 3 ("Install it in the
Absolutely. This should be an option for people to correctly configure their build environment.
Definitely we should add some checks for common cases so as not to mess up a user's environment. Are those the only cases you know of? Would you mind making a list of such cases to check (especially if there are more). |
Any chance we can get this moving again? Should someone step up to finish this or will you have time to finish this one out @msarahan? |
This fix is important if you want CMake to work on Appveyor - with the latest conda-build it shouldn't be required for setuptools packages due to the existence of the Visual C++ for Python package on Appveyor. Perhaps this fix should live in something specific to Appveyor/conda-forge and we should just make sure to have a Wiki page (as @JanSchulz suggested) that explains how to fix this for local/custom builds? |
What about |
If you have some time, @shadowwalkersb , I'd recommend taking a look at this and trying to clean it up. This does basically the same thing as what you were trying to do with |
Please see PR ( #1678 ), which has taken over this. IMHO that is ready to go, but think it is probably healthy for people to take a cursory look at it before it is merged. |
This should replace the 4 lines at https://github.com/conda/conda-build/blob/master/appveyor.yml#L71-L74, and be includable in meta.yaml as a build requirement. Still would be nicer to not need this at all, but I think this is about as clean as we can make it.
cc @patricksnape @jakirkham @pelson @ocefpaf
@patricksnape I added you as a maintainer, since you blazed this trail. Let me know if that's not OK.
License is unknown. What should we put?