Add windows build (try IV) #16

jpfeuffer · 2024-03-28T18:06:14Z

Checklist

Used a personal fork of the feedstock to propose changes
Bumped the build number (if the version is unchanged)
Reset the build number to 0 (if the version changed)
Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
Ensured the license file is being packaged.

…nda-forge-pinning 2023.12.08.09.49.00

…nda-forge-pinning 2024.03.27.17.58.50

conda-forge-webservices · 2024-03-28T18:07:37Z

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.

wolfv · 2024-03-29T09:05:19Z

Hi @jpfeuffer thanks for working on this. If there is anything I can do to help, let me know :)

jpfeuffer · 2024-03-29T13:01:11Z

Thank you! One question, not related to windows. What's happening with the Mac builds? It times out downloading the SDK on Intel for example.

tkralphs · 2024-03-29T15:04:08Z

OK, so now we are back to the original problem, which is that we are trying to link to a BLAS library with Fortran linkage and we need one with C linkage. It looks like we are installing both libblas and libcblas, so maybe don't install libblas. You might also need to check that the path to and name of the library are correct in build.sh. I guess the CBLAS library may have a different name to avoid conflict (libcblas?).

jpfeuffer · 2024-03-29T15:52:28Z

Okay. Now it says it supports C linkage but it "does not work" whatever that means 😅

jpfeuffer · 2024-03-29T16:54:03Z

I found sth that indicates a missing include dir might be the problem:
https://stackoverflow.com/questions/32915936/linking-ipopt-against-openblas

I could add more variables but is there a better/automated way of finding dependencies here?

tkralphs · 2024-03-29T18:07:19Z

The reason for the failure to link is something you would normally figure out by looking at the config.log. If there is a way to get access to build artifacts after the fact, that would help. But I did realize that we are using the flag --with-blas. It might make no difference, but using --with-blas-lib is probably better and just to be safe, you do want to use --with-blas-incdir to specify the right include directory. This is necessary in general when linking to a package for which the header files are not in a location that is in the compiler's header search path.

jpfeuffer · 2024-03-30T11:19:30Z

@tkralphs I am printing the log to the console now. I am not sure what to look for right now. I can go over the whole thing at some point but maybe you will see it quickly.

jpfeuffer · 2024-03-30T16:39:26Z

I actually checked the whole logs now, and there is nothing else hinting on why BLAS linking would fail. But I might have missed something since those logs are incredibly convoluted.

jpfeuffer · 2024-03-30T20:55:53Z

One thing I noticed is that a lot of paths are wrong. Apparently m2 is not well supported in conda and you have to do all kinds of workarounds to preserve paths.

ContinuumIO/anaconda-issues#12124

jpfeuffer · 2024-04-06T09:58:32Z

Ok, as expected, tests introduce another level of complexity.
I would like to merge without tests and move on to the solvers. In the meantime of course someone could do a parallel PR to fix tests here.

tkralphs · 2024-04-06T14:54:11Z

The issue in the test will be fixed in the next release by this: coin-or/CoinUtils#217. Why don't you just leave it for now and after I make the release, we can see if it's fixed.

jpfeuffer · 2024-04-06T15:15:44Z

Yes that's fine, I was about to wait for the release anyway. I just don't want it to be a longer thing because if I lose focus on this side project I am not sure when I will revisit.

tkralphs · 2024-04-06T19:03:32Z

There was a timeout installing Mamba, which I guess will fix itself, but I couldn't see how to force the workflows to be run again except by closing and reopening the PR. But anyway, everything seems to be good with the new release.

wolfv · 2024-04-06T20:25:20Z

I triggered a rerun from the github page :)

jpfeuffer · 2024-04-06T20:27:54Z

I think we need to skip tests on osx-arm since it's cross compiled. Could that be?
Or can you run the executables under Rosetta emulation on those Mac runners?
Edit: Rosetta works the other way round of course. So I think we should disable.

tkralphs · 2024-04-07T00:04:12Z

Yeah, let's just skip tests on osx-arm.

wolfv · 2024-04-07T07:19:32Z

You could also use

test: native_and_emulated

in the conda-forge.yml file and then it should not run the tests on osx-arm64 (after a conda-smithy rerender).

wolfv · 2024-04-07T07:25:17Z

Actually, you can remove the deprecated value of test_on_native_only: true and add test: native_and_emulated to conda-forge.yml and then rerender :)

The rerender would also use the new URL in the readme etc. so it might be good to do anyways.

I did it locally but for some reason can't push to your PR.

jpfeuffer · 2024-04-07T08:47:03Z

@wolfv does this also work if the tests are in the build stage?
I think the failing tests are the one from the build stage. If we want to test the test suite in the test stage we would need to restructure quite a bit.

wolfv · 2024-04-07T09:18:59Z

@jpfeuffer you're right. sorry. you could do a bash if statement in that case.

if [[ $target_platform != "osx-arm64" ]]; then
   make test
fi

h-vetinari · 2024-04-07T13:28:20Z

The general pattern for this would be:

if [[ "$CONDA_BUILD_CROSS_COMPILATION" != "1" ]]; then
  # run the tests; cannot be done in cross-compilation
  make tests
fi

jpfeuffer · 2024-04-07T14:35:47Z

Yep I found it in the docs. I used it like this.
It's ready to merge I think! 🤞

h-vetinari

First look; not sure how this works without a fortran compiler... 🤔

I'm not a maintainer here (though floating around the cbc stack for a while), but personally I'd prefer a cleaner git history for this PR. 🤷

recipe/meta.yaml

h-vetinari · 2024-04-08T02:13:26Z

recipe/meta.yaml

-    - {{ compiler('fortran') }}
-    - pkg-config
-    - make  # [unix]
+    - {{ compiler('fortran') }}  # [not win]


I don't understand how this works - how is it possible that we don't need a fortran compiler on windows after all?
The configure script already warns:

configure: WARNING: Failed to find a Fortran compiler!

The project itself has no Fortran, the Fortran compiler is needed when linking to a Blas/Lapack library with Fortran linkage to determine the name-mangling scheme (I'm not really an expert on this). When either not linking to Blas/Lapack at all (it is not technically needed, though performance is better with it) or when linking to a Blas/Lapack with C linkage, no Fortran compiler is needed. Granted, the warning is a little confusing, though it is only a warning, not an error.

My (now very vague) recollection, pieced together by looking at comments I wrote during earlier attempts, is that we were at first trying to link to Blas/Lapack with Fortran linkage, which first failed because of the lack of a Fortran compiler and then because of the unexpected extension for object files built with flang.

It's quite possible I failed to see the forest for the trees at that point and got caught up in trying to fix the immediate issue rather than taking a step back to see that this could be avoided altogether by just leaving out the --with-blas flag rather than trying to make the Fortran linkage work. I took the presence of those flag for granted for some reason and there's still no way to make the build work with those flags present on WIndows. Even coming back to it this time with fresh eyes, it wasn't until I tried to really isolate why we've able to build on Github Actions and not here that I realized that it was those flags that were the culprit.

Anyway, sorry for the tail-chasing.

h-vetinari · 2024-04-08T02:14:26Z

.ci_support/win_64_.yaml

+fortran_compiler:
+- flang
+fortran_compiler_version:
+- '5'


Note that you're still adding fortran compilers here (seems to be based on an old rerender before adding the selector to the fortran compiler). However, since the dependency is missing in meta.yaml, this isn't actually pulled into the build environment for now.

I would think we should remove this now.

tkralphs · 2024-04-09T02:10:36Z

OK, so everything is passing now, but there are a few unresolved conversations above. @jschueller, can you let me know when you think we're ready to go?

jpfeuffer · 2024-04-09T16:50:30Z

@tkralphs I want to have a look at the blas problem. Can you explain how the version of blas affects coin?
Are there preferred versions? libblas vs libcblas vs none? Does it make sense to have both at any time?

Should we just try to use cblas on all platforms to avoid having to deal with fortran?

tkralphs · 2024-04-10T01:04:55Z

Can you explain how the version of blas affects coin?
Are there preferred versions? libblas vs libcblas vs none? Does it make sense to have both at any time?

As I said, I am not an expert on this, but there are two separate issues. The first issue is practical---how to link to the library. The second issue is performance. There are many different implementations of the BLAS API and their performance will differ. Which one would be the best performing with Cbc probably differs by platform. I don't have a specific recommendation for that. On Windows, whatever we can make work would be good.

For the linking, to link to a library with Fortran linkage, you need to know the name-mangling scheme used by the library (you can read about the general idea here if you're unfamiliar), which is why the Fortran compiler is required. My recollection, though. is that flang is not going to work as a compiler on Windows because it doesn't produce object files with the extension expected on Windows (.o versus .obj, unless this has changed). So even if the linking is successful, the tests fails because configure does not detect that the object files was correctly produced.

So I believe our only option is to link to a library with C linkage. cblas is just a C wrapper around the original BLAS interface and should be what we need. But I could not see how to link to the DLL provided by cblas, as discussed above.

That is about the extent of my knowledge. We can seek out some advice if we need to know more, but I would personally just merge and move on to trying to get a working Cbc on Windows. After it's working without BLAS, we can separately try to add BLAS later.

…os c17

jpfeuffer · 2024-04-10T10:01:19Z

Ok since we are also not passing --with-lapack and lapack also needs fortran, I removed everything fortran related now.
Let's add it coorectly in a second round.

jpfeuffer · 2024-04-10T10:01:58Z

Will do a re-render once it passes.

jpfeuffer · 2024-04-10T11:17:43Z

@conda-forge-admin, please rerender

github-actions · 2024-04-10T11:19:44Z

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like I wasn't able to push to the add-windows-build branch of jpfeuffer/coin-or-utils-feedstock. Did you check the "Allow edits from maintainers" box?

NOTE: Our webservices cannot push to PRs from organization accounts or PRs from forks made from organization forks because of GitHub permissions. Please fork the feedstock directly from conda-forge into your personal GitHub account.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/coin-or-utils-feedstock/actions/runs/8630351073.

jpfeuffer · 2024-04-10T12:16:29Z

I re-did the PR by re-forking from conda-forge directly. Also to cleanup commit history.

jpfeuffer · 2024-04-10T12:21:22Z

That is about the extent of my knowledge. We can seek out some advice if we need to know more, but I would personally just merge and move on to trying to get a working Cbc on Windows. After it's working without BLAS, we can separately try to add BLAS later.

I am fine with that approach. However, this might be surprising for some people used to having this feature. At least during the transition period until our new release is on par with the old one again.

brynpickering and others added 3 commits December 8, 2023 16:16

Test old PR implementation of windows build

b24d6fb

MNT: Re-rendered with conda-build 3.28.1, conda-smithy 3.30.1, and co…

573bf0b

…nda-forge-pinning 2023.12.08.09.49.00

MNT: Re-rendered with conda-build 24.3.0, conda-smithy 3.34.0, and co…

2a5bd96

…nda-forge-pinning 2024.03.27.17.58.50

jpfeuffer requested review from jschueller, tkralphs and wolfv as code owners March 28, 2024 18:06

Update build.sh

0386e02

jpfeuffer added 2 commits March 28, 2024 20:08

Update build.sh

e187caf

Update meta.yaml

90d6c3a

jpfeuffer mentioned this pull request Mar 28, 2024

Provide cbc for Conda on Windows conda-forge/coin-or-cbc-feedstock#3

Open

1 task

Use M2 make

5d713a0

jpfeuffer added 2 commits March 29, 2024 14:46

try without Flang on win

9cf3964

no Fortran variables also

621b6f1

try cblas. no idea if the lib is called like this

6564439

jpfeuffer added 3 commits March 30, 2024 10:22

do not install non-C blas on windows

4d45abc

add some guessed include dir. hoping it works without quotes

0b0d86f

try cat config.log

7630df2

tkralphs and others added 2 commits March 30, 2024 14:03

cat the config.log in the subdirectory

703367c

Use slash

92d2a14

Try just letting configure find the BLAS library on its own

3956879

add make test

b469f83

tkralphs added 2 commits April 6, 2024 14:15

Bump version

ca04f50

Update SHA

219db87

do not make test when cross compiling

4a19149

h-vetinari reviewed Apr 8, 2024

View reviewed changes

remove all mentions of fortran, blas, lapack; address review; try mac…

520a6d5

…os c17

jpfeuffer closed this by deleting the head repository Apr 10, 2024

jpfeuffer mentioned this pull request Apr 10, 2024

Update to 2.11.11 and add windows build #18

Merged

5 tasks

Add windows build (try IV) #16

Add windows build (try IV) #16

Conversation

jpfeuffer commented Mar 28, 2024 • edited Loading

conda-forge-webservices bot commented Mar 28, 2024

wolfv commented Mar 29, 2024

jpfeuffer commented Mar 29, 2024

tkralphs commented Mar 29, 2024

jpfeuffer commented Mar 29, 2024

jpfeuffer commented Mar 29, 2024

tkralphs commented Mar 29, 2024 • edited Loading

jpfeuffer commented Mar 30, 2024

jpfeuffer commented Mar 30, 2024

jpfeuffer commented Mar 30, 2024

jpfeuffer commented Apr 6, 2024

tkralphs commented Apr 6, 2024

jpfeuffer commented Apr 6, 2024

tkralphs commented Apr 6, 2024 • edited Loading

wolfv commented Apr 6, 2024

jpfeuffer commented Apr 6, 2024 • edited Loading

tkralphs commented Apr 7, 2024

wolfv commented Apr 7, 2024

wolfv commented Apr 7, 2024

jpfeuffer commented Apr 7, 2024

wolfv commented Apr 7, 2024

h-vetinari commented Apr 7, 2024

jpfeuffer commented Apr 7, 2024

h-vetinari left a comment

Choose a reason for hiding this comment

h-vetinari Apr 8, 2024

Choose a reason for hiding this comment

tkralphs Apr 8, 2024

Choose a reason for hiding this comment

h-vetinari Apr 8, 2024

Choose a reason for hiding this comment

tkralphs Apr 8, 2024

Choose a reason for hiding this comment

tkralphs commented Apr 9, 2024

jpfeuffer commented Apr 9, 2024 • edited Loading

tkralphs commented Apr 10, 2024

jpfeuffer commented Apr 10, 2024 • edited Loading

jpfeuffer commented Apr 10, 2024

jpfeuffer commented Apr 10, 2024

github-actions bot commented Apr 10, 2024

jpfeuffer commented Apr 10, 2024

jpfeuffer commented Apr 10, 2024

jpfeuffer commented Mar 28, 2024 •

edited

Loading

tkralphs commented Mar 29, 2024 •

edited

Loading

tkralphs commented Apr 6, 2024 •

edited

Loading

jpfeuffer commented Apr 6, 2024 •

edited

Loading

jpfeuffer commented Apr 9, 2024 •

edited

Loading

jpfeuffer commented Apr 10, 2024 •

edited

Loading