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

Vs2008 appveyor cmake #617

Closed
wants to merge 2 commits into from

Conversation

msarahan
Copy link
Member

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?

@conda-forge-linter
Copy link

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 (recipes/vs2008_express_vc_python_patch) and found some lint.

Here's what I've got...

For recipes/vs2008_express_vc_python_patch:

  • The home item is expected in the about section.
  • The license item is expected in the about section.

@jakirkham
Copy link
Member

License is unknown. What should we put?

In the past we have done BSD 3-Clause (e.g. vc). Seems ok to do the same here. Not sure what others think.

version: 1.0

source:
url: https://github.com/menpo/condaci/raw/master/vs2008_patch.zip
Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.)

Copy link
Member

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?

Copy link
Member

@jakirkham jakirkham Jul 2, 2016

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?

Copy link
Contributor

@patricksnape patricksnape Jul 7, 2016

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.

Copy link
Member

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.

Copy link
Member

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.

@pelson
Copy link
Member

pelson commented May 14, 2016

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?

@patricksnape
Copy link
Contributor

Yeah if you install this as a build-dep it will fix Appveyor - very smart idea @msarahan!

@patricksnape
Copy link
Contributor

We probably only want to run it on x64?

@jakirkham
Copy link
Member

jakirkham commented May 18, 2016

So, there are a few ways I could see one going with this.

  1. Make it a build dependency of recipes on Windows.
  2. Make this a run time dependency of the toolchain ( Add a recipe to configure our toolchain #578 ) on Windows.
  3. Install it in the root environment in the AppVeyor CI scripts.

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.

@pelson
Copy link
Member

pelson commented May 18, 2016

  1. Install it in the root environment in the AppVeyor CI scripts.

👍 - 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"
Copy link
Member

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?

@jakirkham
Copy link
Member

cc @JanSchulz

@jankatins
Copy link
Contributor

Just for reference:Make it a build dependency of recipes on Windows. is IMO a nogo: copying to C:\Program Files (x86) is restricted to admin rights and I don't want to run local builds with admin rights.

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 "C:\Program Files (x86)\Microsoft Visual Studio 9.0\VC\bin\vcvars64.bat", only

C:\Users\jschulz\AppData\Local\Programs\Common\Microsoft\Visual C++ for Python\9.0\VC\bin\vcvars64.bat
# [and the one from 10.0 and 14.0 under C:\Program Files (x86)\]

I do have:

C:\Program Files (x86)\Microsoft Visual Studio 9.0\VC\bin\amd64\vcvarsamd64.bat

@jakirkham
Copy link
Member

Just for reference:Make it a build dependency of recipes on Windows. is IMO a nogo: copying to C:\Program Files (x86) is restricted to admin rights and I don't want to run local builds with admin rights.

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 root environment in the AppVeyor CI scripts.") would be the way to go. Though may be that we do this via conda-forge-build-setup at conda-forge.

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.

Absolutely. This should be an option for people to correctly configure their build environment.

...some checks could be added...

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).

@jakirkham
Copy link
Member

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?

@patricksnape
Copy link
Contributor

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?

@jakirkham
Copy link
Member

What about conda-forge-build-setup?

@jakirkham
Copy link
Member

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 freeglut. Though it would be a package one could install to make Python 2.7/VS 2008 64-bit builds work correctly.

xref: conda-forge/freeglut-feedstock#2 (diff)

@jakirkham
Copy link
Member

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.

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

Successfully merging this pull request may close these issues.

6 participants