search

conda-forge / suitesparse-feedstock

A conda-smithy repository for suitesparse.
BSD 3-Clause "New" or "Revised" License
1 stars 16 forks source link

Windows first try #30

Closed ghost closed 6 years ago

ghost commented 6 years ago

Fixes https://github.com/conda-forge/suitesparse-feedstock/issues/4

Moving forward here because this won't override any existing suitesparse.

conda-forge-linter commented 6 years ago

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

ghost commented 6 years ago

@conda-forge-admin Please rerender.

ghost commented 6 years ago

@conda-forge-admin Please rerender

ghost commented 6 years ago

@conda-forge-admin Please rerender.

ghost commented 6 years ago

Any idea how to implement the file tests on Windows?

ghost commented 6 years ago

@jakirkham This is ready to go.

jakirkham commented 6 years ago

Would you be alright adding yourself as a maintainer @xoviat? I don't think I have the bandwidth to maintain the Windows side of this correctly.

ghost commented 6 years ago

@jakirkham Done.

ghost commented 6 years ago

Ready to merge?

jakirkham commented 6 years ago

So since the Windows version is different, I'd actually prefer to put this in a different branch other than master. That way we can build Unix and Windows for that version. As it's kind of late, here I'll look at it tomorrow.

ghost commented 6 years ago

@jakirkham Can you create the branch?

jakirkham commented 6 years ago

Done. It pulled in a lot of history as a consequence of where the branch was placed. May want to squash some commits here. Also put PR ( https://github.com/conda-forge/suitesparse-feedstock/pull/31 ) up to re-render that branch.

ghost commented 6 years ago

@conda-forge-admin Please rerender

conda-forge-linter commented 6 years ago

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict. Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

jakirkham commented 6 years ago

Toggling to see if that will fix the now re-rendered branch.

jakirkham commented 6 years ago

@conda-forge-admin, please rerender.

conda-forge-linter commented 6 years ago

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

jakirkham commented 6 years ago

Fixed the conflicts. However it looks like Unix builds were disabled. Also the version appears to still be incorrect. Could you please correct these when you have a chance?

ghost commented 6 years ago

See my other comment: master should build linux and osx, while 'windows' should build windows. Reason: they have different release cycles.

jakirkham commented 6 years ago

I’m starting to wonder if these wouldn’t be better off as two different packages. Partially because of the versioning point you have made and partially because that fork also builds on Unix. Not sure how we resolve conflicts between them if a user tries to install both.

ghost commented 6 years ago

No. It's the same package that provides the same functionality. You should simply create another branch that builds windows only or add me as a maintainer so that I can do it. The only disadvantage of the windows package is that it builds an older version.

ghost commented 6 years ago

Having a different package is just going to make people have to put conditions into their batch files and unnecessarily complicate the user experience. There is no reason to do this.

isuruf commented 6 years ago

Maybe we can do what vcpkg does. vcpkg downloads the fork with CMake files and replaces the Suitesparse source with the version needed (4.5.5 in vcpkg case)

ghost commented 6 years ago

Right now we have a situation where this package is not available on Windows. What I am suggesting is we make an older version available on Windows, which would address almost everyone's concerns. If people want a newer version, then we can take care of that later but I'm not sure that the benefit from that justifies the effort.

jakirkham commented 6 years ago

We could also just vendor the CMake stuff in the recipe and use the same source for both. There are many recipes exactly like this in conda-forge.

ghost commented 6 years ago

I don't know that the CMake files will build with the latest version. I know that they will build with this version. I also don't think that people will need a newer version on Windows at this time, so we should wait until either someone requests it, or wait until the CMake author updates their sources.

ghost commented 6 years ago

I assume this is not moving forward. I am not exactly sure why it is better to have no Windows build.

grlee77 commented 6 years ago

If I understand correctly, what @xoviat is proposing is that we only supply this version on Windows in a separate windows branch and will not tend to update it unless someone who is motivated and willing to help do so comes along. There would be no linux/osx package for this particular version, so it makes less sense to have the branch named v4.5.1.

I think that is a reasonable first step to Windows support. Later if someone has the time/inclination to improve the windows packages, we can always update then.

ghost commented 6 years ago

Yes, that's almost exactly what I'm saying.

I think that is a reasonable first step to Windows support. Later if someone has the time/inclination to improve the windows packages, we can always update then.

One particular fact to note is that the effort for this is not spread across conda-forge, but conda-forge, vcpkg, and others. All that's required is to update the source repository.

isuruf commented 6 years ago

We could also just vendor the CMake stuff in the recipe and use the same source for both

It's a lot of files, so I'm +1 to using a separate branch for windows.

ghost commented 6 years ago

@conda-forge-admin Please rerender.

ghost commented 6 years ago

So I'm just going to merge this in a week or so if there are no additional comments.

jakirkham commented 6 years ago

Sorry have been in and out of meetings/appointments for the past couple days. I'm generally ok with Windows being here in some form. Was hoping that we would be able to have a little more parity between OSes and my suggestions had this in mind. However if that is not possible, I understand and it shouldn't be blocking. Feel free to do what you think is best.

jakirkham commented 6 years ago

If you have time to take a look @patricksnape, would be great to have your feedback. If not, no worries.

patricksnape commented 6 years ago

I'm a bit out of the loop here sorry - looks like all the Unix build files were deleted and we are proposing to place this on a branch - but I can't really gather why. There doesn't actually seems to be that much windows specific code (to me). Seems like if we go this route then that branch will probably be quickly orphaned.

Also - does the vc14 feature prevent this from being installed on Python 2 where the wrong compiler would otherwise be used?

ghost commented 6 years ago

So I'm not going to rehash the entire discussion here, and I don't think that there are going to be any more comments. I said a week, but I don't think there's a reason to wait longer.

jakirkham commented 6 years ago

Generally had the same feelings initially @patricksnape. However it seems it is too hard to keep the Windows fork of SuiteSparse up to date with the original Unix copy of SuiteSparse. Hence the separate branch. FWIW that Windows fork is the recommended way to install SuiteSparse on Windows by the author of SuiteSparse.

Yes the vc14 feature is set by Python 3.5 and 3.6 (not Python 2.7). So this should only install when one of those is present.

patricksnape commented 6 years ago

Sounds great, thanks for the update