Vs2008 appveyor cmake #1678

shadowwalkersb · 2016-09-28T05:31:56Z

Closes #617
Closes #1737

There isn't much I changed, but this is based on PR ( #617 ).

conda-forge-linter · 2016-09-28T05:31:59Z

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.

shadowwalkersb · 2016-09-28T05:34:05Z

recipes/vs2008_express_vc_python_patch/meta.yaml

+  version: 1.0
+
+source:
+  url: https://github.com/conda-forge/conda-forge-build-setup-feedstock/raw/master/vs2008_patch.zip


This, obviously, needs to be added to conda-forge-build-setup-feedstock if approved.

We need this file.

xref: https://github.com/menpo/condaci/blob/98d76b2089d494433ac2e3abd920123088a95a6d/vs2008_patch.zip

Though we talked about this before and I think we are ok extracting and including that stuff here.

Should I upload it to conda-forge-build-setup-feedstock via a PR?

Also please see these, which are basically the same.

xref: #251
xref: conda-forge/conda-smithy#107

Should I upload it to conda-forge-build-setup-feedstock via a PR?

Nope. We should decompress it and include its source in this recipe.

shadowwalkersb · 2016-09-28T05:35:32Z

I am not sure how to address this, #617 (comment).

jakirkham · 2016-09-28T05:36:52Z

appveyor.yml

-    - TARGET_ARCH: "x64"
-      CONDA_PY: "35"
-      PY_CONDITION: "python >=3.5"
-      CONDA_INSTALL_LOCN: "C:\\Miniconda35-x64"


So this stuff shouldn't be changed right?

Wasn't sure. Reverting.

This reverts commit 9e30b8a.

jakirkham · 2016-09-28T05:41:40Z

I am not sure how to address this, #617 (comment).

I think this will get a lot easier once we include the source in the recipe. Then we can tweak the script and the like.

jakirkham · 2016-09-28T05:43:48Z

cc @patricksnape

jakirkham · 2016-09-28T13:24:21Z

appveyor.yml

@@ -72,6 +72,9 @@ install:
    # Remove cygwin (and therefore the git that comes with it).
    - cmd: rmdir C:\cygwin /s /q

+    # Install workaround 64-bit support for VS 2008.
+    - cmd: if "%TARGET_ARCH%" == "x64" if "%CONDA_PY%" == "27" call vs2008\setup_x64.bat


Please remove this. The point of this package is it will take its place ultimately.

This reverts commit 7ed8bf9.

* Include the `x64` directory of reg files from staged-recipes history. * Include `setup_x64.bat` script from staged-recipes history. * Remove `post-link.bat` and incorporate relevant pieces. * Update `meta.yaml` to handle current installation structure. * Drop `bld.bat` as the script is included in `meta.yaml` to be compact. * Revert changes made to `appveyor.yml` as they are not needed.

jakirkham · 2016-09-28T14:17:16Z

Please see PR ( https://github.com/shadowwalkersb/staged-recipes/pull/1 ). This may help get this on the right track. May still need some more fixes, but we can find out after it is merged.

Restructure vs2008_express_vc_python_patch to include source.

conda-forge-linter · 2016-09-28T14:20:10Z

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 (recipes/vs2008_express_vc_python_patch) and found it was in an excellent condition.

jakirkham · 2016-09-28T14:23:46Z

recipes/vs2008_express_vc_python_patch/meta.yaml

+  script:
+    - if note exists %SCRIPTS% mkdir %SCRIPTS%
+    - xcopy /v %RECIPE_DIR%\\setup_x64.bat %SCRIPTS%\\setup_x64.bat
+    - xcopy /v %RECIPE_DIR%\\vs2008 %SCRIPTS%


As things got moved around in interim, will need to change ``%RECIPE_DIR%->%RECIPE_DIR%\vs2008_patch`.

jakirkham · 2016-09-28T14:34:16Z

So a bunch of files got moved around in commit ( fb29951 ). I would revert the move as things are a bit dependent on those locations ATM.

jakirkham · 2016-09-28T14:35:33Z

Alternatively one could move the rest of the stuff to vs2008_patch and update the paths in meta.yaml to handle this.

shadowwalkersb · 2016-09-28T14:36:16Z

Do you want to keep vs2008_patch or move up setup_x64 and x64?

shadowwalkersb · 2016-09-28T14:37:44Z

Do you want to handle the restructuring? It may be easier. We keep overlapping.

jakirkham · 2016-09-28T19:13:54Z

My concern here is that the user may not be aware of the ramifications of "installing" this package.

This is why the post link script was decided to be too helpful. Hence we removed it.

It goes beyond installing a package, but modifies the local system. It is "Are you really sure you want to this?" kind of thing, but there is no way of warning the user or prompting. Or, maybe there is?

Ah, but there is no post link script any more. The user must not only install this package, but run the script setup_x64.bat. So this feels like a pretty intentional action IMHO. We also now have the Readme that explains a bit about what happens. That being said, we have many options for improvement.

Add more info to the Readme.
Add a prompt for running the script too.
Add a script to revert the patch.

This all being said, I don't see any of these as blockers to merging this. They all seem like things that could be opened as issues on the feedstock.

jakirkham · 2016-09-28T19:20:02Z

recipes/vs2008_express_vc_python_patch/meta.yaml

@@ -0,0 +1,26 @@
+package:
+  name: vs2008_express_vc_python_patch
+  version: 1.0


Might make it 1.0.0.

jakirkham · 2016-09-28T19:20:28Z

recipes/vs2008_express_vc_python_patch/meta.yaml

+extra:
+  recipe-maintainers:
+    - msarahan
+    - patricksnape


Any interest in being a maintainer, @shadowwalkersb?

Probably should add me too.

Done below. Thanks.

I can always submit a PR to be removed, right? ;-)

I suppose. 😢

To be honest though, I don't expect this to see lots of activity. It basically works. @patricksnape put a lot of work in getting this put together right.

shadowwalkersb · 2016-09-28T19:26:38Z

This is why the post link script was decided to be too helpful. Hence we removed it.

That makes sense.

Ah, but there is no post link script any more. The user must not only install this package, but run the script setup_x64.bat.

Was missing that point. Now, it makes more sense.

Thanks!

jakirkham · 2016-09-28T19:28:20Z

Also forgot to note that running that script basically requires admin privileges. So that question will certainly be asked too.

jakirkham · 2016-09-28T19:31:29Z

Copying all of you as you were all involved in the discussion in PR ( #617 ). This takes over that PR and addresses many of the concerns raised by many of you over there. Would be great if you could take a quick look and let us know if it seems ok. Then we can get this merged and integrated into our CI builds. Thanks in advance.

cc @msarahan @pelson @patricksnape @JanSchulz

patricksnape · 2016-09-28T19:53:19Z

recipes/vs2008_express_vc_python_patch/meta.yaml

+
+build:
+  number: 0
+  skip: true  # [not win]


I wonder if we should only run this on Appveyor? We could add something like # [not win and environ['APPVEYOR'] != "True"]. This would prevent someone from downloading a recipe and having it mess with their system.

Why would it mess with their system? We dropped the post link script.

If someone relies on this package, presumably they will run the setup_x64.bat? This then messes with the users system?

Sure, it does, but couldn't that be desirable?

For instance, I have an old Windows computer that I would like to apply this patch to so that I can build 64-bit VS 2008 binaries with. Is there any reason I should not do that?

For a start it will fail if you run it twice (I'm pretty sure). Secondly, it will only run as Administrator and so may fail fairly cryptically if you aren't running in an elevated prompt. Otherwise, no.

Ok, so am not too worried about these ATM. Though I think these are all things that could be raised at the feedstock. Here were a few thoughts along these lines.

patricksnape · 2016-09-28T19:55:08Z

I'm not actually sure why a lot of my conda-build changes got reverted. But this certainly helps on Appveyor for CMake x64 builds. It still remains to be a 'caveat' that people need to know it exists, but it could be useful to point people towards.

jakirkham · 2016-09-28T19:59:58Z

While you're here @patricksnape, we found some 32-bit reg files in that zip, but we dropped them (IIRC we never used/had them at conda-forge). Are they actually needed for something?

patricksnape · 2016-09-28T20:02:54Z

It's only for Itanium builds. In fact, I'm not sure you even need the reg files at all - I've never needed them for actual x86_64.

jakirkham · 2016-09-28T20:08:33Z

Hmm...I thought that was necessary for Visual Studio to find VC Express, no?

patricksnape · 2016-09-28T20:20:05Z

I think you need the last 5 ones that mention AMD64 - but not the Itanium ones.

jakirkham · 2016-09-28T20:51:57Z

So the ones beginning with 600* are IA64 and the ones beginning with 656* are AMD64. However, VC_OBJECTS_PLATFORM_INFO.reg refers to both. Am a little nervous dropping those files if they will still be referred to by VC_OBJECTS_PLATFORM_INFO.reg and don't know enough to safely edit VC_OBJECTS_PLATFORM_INFO.reg. Would it hurt to just leave them? Or do you know how you want these modified?

patricksnape · 2016-09-29T08:13:01Z

I don't think it really matters - was just pointing out that there are probably more than the required number of files here. You can likely leave this as is.

jakirkham · 2016-10-04T01:38:45Z

Thoughts on this, @pelson @msarahan ?

patricksnape · 2016-10-04T18:13:48Z

It's all good from me if it seems to fix the cmake issues. Maybe we could add a dummy cmake build for VS2008 x64 to check it actually fixes the issue?

jakirkham · 2016-10-09T17:39:50Z

Please see PR ( #1737 ), which tests this VS 2008 64-bit strategy and demonstrates the problem and that it fixes it.

Edit: Added a closes note in the OP to close this testing PR when we merge this PR.

jakirkham · 2016-10-09T17:40:58Z

Would love if @msarahan and @pelson could comment. Though at this point I'm tempted to just merge this as it is a working usable strategy that we can already put into play.

patricksnape · 2016-10-09T18:08:27Z

Thanks for the dummy build @jakirkham! I'm happy with this to go in now!

msarahan

Looks fine to me. This probably only works on appveyor and other places where people have admin rights. That shouldn't prevent it from getting out there, it should just be a known limitation.

msarahan and others added 4 commits September 28, 2016 00:23

add script to set up VS 2008 for CMake on appveyor

01441b3

whitespace

117be6c

download tarball from conda-forge/conda-forge-build-setup-feedstock

ef8f01c

build for win x64 only

9e30b8a

shadowwalkersb commented Sep 28, 2016

View reviewed changes

jakirkham reviewed Sep 28, 2016

View reviewed changes

Revert "build for win x64 only"

2e067a7

This reverts commit 9e30b8a.

shadowwalkersb added 4 commits September 28, 2016 00:53

add vs2008_patch.zip

56cf881

update download url

c0baccb

fix download path

d017c2d

add workaround code to appveyor.yml

7ed8bf9

jakirkham reviewed Sep 28, 2016

View reviewed changes

jakirkham mentioned this pull request Sep 28, 2016

Fix VC9 Win64 build on AppVeyor conda-forge/conda-smithy#295

Closed

1 task

shadowwalkersb and others added 4 commits September 28, 2016 09:00

Revert "add workaround code to appveyor.yml"

d9713cc

This reverts commit 7ed8bf9.

decompress patch contents

76d0a61

move patch files into recipe

fb29951

Merge pull request #1 from jakirkham/vs2008_appveyor_cmake__inc_reg

6b0a6c9

Restructure vs2008_express_vc_python_patch to include source.

jakirkham reviewed Sep 28, 2016

View reviewed changes

fix typos

f491334

jakirkham reviewed Sep 28, 2016

View reviewed changes

jakirkham mentioned this pull request Sep 28, 2016

Vs2008 appveyor cmake #617

Closed

update version # and add maintainers

7647e5b

jakirkham approved these changes Sep 28, 2016

View reviewed changes

patricksnape reviewed Sep 28, 2016

View reviewed changes

Luthaf mentioned this pull request Sep 30, 2016

Add recipes for Chemfiles #1571

Merged

jakirkham mentioned this pull request Oct 9, 2016

NO MRG: TST: Try using VS2008 AppVeyor CMake #1737

Closed

jakirkham mentioned this pull request Oct 9, 2016

Collections of recipes needing attention? #161

Closed

msarahan approved these changes Oct 9, 2016

View reviewed changes

patricksnape merged commit 90be239 into conda-forge:master Oct 10, 2016

shadowwalkersb deleted the vs2008_appveyor_cmake branch October 12, 2016 00:44

msarahan mentioned this pull request Jan 2, 2017

Windows compiler not properly activated on python 2.7/64-bit conda/conda-build#1606

Closed

jakirkham mentioned this pull request Jan 7, 2017

Install vs2008_express_vc_python_patch on appveyor conda-forge/conda-forge-build-setup-feedstock#48

Merged

Vs2008 appveyor cmake #1678

Vs2008 appveyor cmake #1678

Conversation

shadowwalkersb commented Sep 28, 2016 • edited by jakirkham Loading

conda-forge-linter commented Sep 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shadowwalkersb commented Sep 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented Sep 28, 2016

jakirkham commented Sep 28, 2016

Choose a reason for hiding this comment

jakirkham commented Sep 28, 2016

conda-forge-linter commented Sep 28, 2016

Choose a reason for hiding this comment

jakirkham commented Sep 28, 2016

jakirkham commented Sep 28, 2016

shadowwalkersb commented Sep 28, 2016

shadowwalkersb commented Sep 28, 2016

jakirkham commented Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

jakirkham Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

shadowwalkersb commented Sep 28, 2016 • edited Loading

jakirkham commented Sep 28, 2016

jakirkham commented Sep 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricksnape commented Sep 28, 2016 • edited Loading

jakirkham commented Sep 28, 2016 • edited Loading

patricksnape commented Sep 28, 2016

jakirkham commented Sep 28, 2016 • edited Loading

patricksnape commented Sep 28, 2016

jakirkham commented Sep 28, 2016

patricksnape commented Sep 29, 2016

jakirkham commented Oct 4, 2016

patricksnape commented Oct 4, 2016

jakirkham commented Oct 9, 2016 • edited Loading

jakirkham commented Oct 9, 2016 • edited Loading

patricksnape commented Oct 9, 2016

msarahan left a comment

Choose a reason for hiding this comment

shadowwalkersb commented Sep 28, 2016 •

edited by jakirkham

Loading

jakirkham commented Sep 28, 2016 •

edited

Loading

jakirkham Sep 28, 2016 •

edited

Loading

jakirkham Sep 28, 2016 •

edited

Loading

shadowwalkersb commented Sep 28, 2016 •

edited

Loading

patricksnape commented Sep 28, 2016 •

edited

Loading

jakirkham commented Sep 28, 2016 •

edited

Loading

jakirkham commented Sep 28, 2016 •

edited

Loading

jakirkham commented Oct 9, 2016 •

edited

Loading

jakirkham commented Oct 9, 2016 •

edited

Loading