search

coin-or / COIN-OR-OptimizationSuite

A harness for building the bundled suite of interoperable optimization tools available in the COIN-OR repository.
116 stars 41 forks source link

Windows-ci actions mingw32 fail for Clp and Cbc because metis package not found #25

Closed jhmgoossens closed 5 months ago

jhmgoossens commented 8 months ago

Since ~ 14 DEC 2023, the mingw32 windows-ci actions for Clp and Cbc fail with 

C:\msys64\usr\bin\pacman -S mingw-w64-i686-lapack mingw-w64-i686-winpthreads-git mingw-w64-i686-readline mingw-w64-i686-suitesparse mingw-w64-i686-metis --noconfirm
[..]
error: target not found: mingw-w64-i686-metis

This comes up in open PRs such as CBC PR 630.

This still works fine for mingw64 builds where this resolves to the mingw-w64-x86_64-metis-5.1.0-4 package. I'm not familiar with pacman under msys2, but indeed the metis (and lapack) package is not listed on the mingw32 package list but is listed on the mingw64 list.

Any suggestions how to fix this? Get the package from somewhere else? We could consider retiring the Windows 32-bit builds, or building them without Metis? The Windows MSVS and MSVC builds already don't use metis, though that is not a feature.

jhmgoossens commented 7 months ago

Hi @tkralphs This issue of missing metis and lapack for msys2 for mingw32 (for Windows) is still breaking the Windows-ci builds for Cbc, Osi, etc. The easiest way around is to simply drop mingw32 32-bit Windows builds from the matrix in Windows-ci--I don't believe these are much used anyway, but I could be wrong. Thoughts? (This issue should be labelled "gh-actions")

jhmgoossens commented 7 months ago

See also MSYS2 - Starting to drop some 32-bit packages and Cygwin x86 end-of-life. Indeed, I propose to update the windows-ci steps to follow the recommendation to "Switch your workflow to use 64-bit packages instead ;)" and thus to remove the two mingw32 matrix entries.

tkralphs commented 7 months ago

Sorry for the delay. Sure, dropping 32-bit support seems to make sense. How urgent is this? It would be good to take a look at the whole picture and sync everything up. I have the feeling that some one-off stuff has snuck in over the last year or so and I wanted to try to sync everything up. Also, there are some other weird deprecations and failures lurking, though I can't recall right now what they are. I would need to find a little time to do all that. We can also partner on that if you want. If it's urgent and you just want a quick fix, I don't object to doing PRs for just the Cbc stack.

jhmgoossens commented 7 months ago

Not urgent for me--This mingw32 issue leads to failed red x's in the checks of PRs. Not sure if PRs are used a lot in coin-or, but soon it'll just be normal to merge PRs with failed checks, and all will go down the drain fast after that (a little overdramatic..)

some one-off stuff [..] other weird deprecations and failures

Happy to help. I see there are like 11 projects with linux/windows-ci workflows in coin-or. I'll have a look at recent runs and compare masters and current stables. Something like that?

tkralphs commented 7 months ago

Happy to help. I see there are like 11 projects with linux/windows-ci workflows in coin-or. I'll have a look at recent runs and compare masters and current stables. Something like that?

Yes! Look at the recent runs and try to spot any deprecation warnings, etc. I think there may be some images that are being sunsetted that we are still using and I know there are some actions that are no longer maintained that we should move away from (off the top of my head, there is one that uploads assets to releases, I think?). But also look at differences between the config.yml for stable versions and master versions across projects. Ideally, they would all be identical, which makes maintenance easier. I think there are some small difference that are unavoidable, but what I'm looking for are things I fixed in one project where the fix should be applied to all others and I just never got to it. Stuff like that.

jhmgoossens commented 7 months ago

I've firstly compared the various linux-ci. Many obvious differences that can be harmonized, such as the outdated OS matrix.

But I'm not so sure about the following. Added my proposals. Comments?

General:

Cbc:

Cgl:

DyLP:

SYMPHONY:

jhmgoossens commented 6 months ago

@tkralphs Any comments for the linux-ci harmonization proposals?

tkralphs commented 6 months ago

Oops, sorry that I missed this!

I'm not so sure about the following. Added my proposals. Comments?

General:

  • "visibility=hidden": Many matrix entries use "flags: [ADD_CXXFLAGS=-fvisibility=hidden]", but often the stable workflow doesnt contain this.

    • Proposal: Add flags: [ADD_CXXFLAGS=-fvisibility=hidden] to master and stable.

      We only do this in master and not in stables because we previously did not have the compiler directives needed to expose only the parts of the interface we want, so the result is that everything is hidden in stable. Nothing would build correctly with this option.

  • "--disable-shared": Most workflows use the same "ADD_BUILD_ARGS+=( --static --with-lapack='-llapack -lblas -lgfortran -lquadmath -lm' )", but MibS and SYMPHONY use "ADD_BUILD_ARGS+=( --disable-shared )".

    • Proposal: Use the same "--static --with-lapack..." also in MibS and SYMPHONY

      There was a reason for this (see https://github.com/coin-or/MibS/commit/28cd0e6f9ce9eda4f2e4d793326e81f499cda8a2), but I think it amounted to not being able to get a fully static build to work and jusr running out of time and patience for it. I think things will break if we unify this, but we could see whether it's possible to fix it.

  • head_ref: MibS stable has a fix for filtered_head_ref to remove slashes from head ref for upload artifact name

    • Proposal: Good point, but can be resolved by removing the head_ref from the asset name altogether. This should be used in all workflows.

      I don't know why this was never an issue for other projects. If you look at the file names for the releases of those projects, they already have / replaced with ., so the action itself must have changed and we just haven't made any releases since that change (other than MibS). I do recall now, though, that when I was poking at this to see what happened, I happened to see that actions/upload-release-asset is now deprecated and there are some better options. This is one of the things that I thought should be changed and this may fix the issue as well. I'm not sure what you mean by not having head_ref as part of the filename, though. You mean just the version number and not releases/? Then I guess you still need to filter to get the version number. The version number should be there I think, right?

Cbc:

  • Stable uses arch=auto in all macos-13 matrix entries, but master doesn't add arch=auto

    • Proposal: Remove, seems redundant

      I was playing around with seeing if I could get builds working on arm-based OS X. I see conflicting information out there about whether you can do this. I saw the arch keyword used somewhere in another project on OS X and I was playing around with it, but I finally ran out of time and gave up. So OK, let's remove it for now.

  • Stable for ubuntu builds add nauty settings [[ ${{ matrix.os }} == ubuntu* ]] && ADD_ARGS+=( --with-nauty-incdir=/usr/include/x86_64-linux-gnu/nauty --with-nauty-lib="-L/usr/lib/ -lnauty" )

    • Proposal: Remove? Not in master, so not in stable?

      Well, we really should be testing with nauty I think. It probably doesn't hurt to add it everywhere to keep uniformity and it will hopefully just get ignored.

Cgl:

  • Master uses two macos-13 for CC=clang

    • Proposal: Remove second macos-13 for CC=clang, keeping only the first macos-13 for CC=clang

      There was some weirdness going on where some unit tests were failing only on very specific combinations of OSX version and compiler. I eventually just removed some problematic one I think and this may have happened when I was playing around with that. It would be nice to sort out, but no time.

  • Stable doesn’t upload assets for arm64 (" github.include.arch != 'arm64' ")

    • Proposal: Remove part of "if". Upload assets anyway

      Hmm, I'm not sure, but this probably had to do with trying to get arm builds working on OSX. So yeah, we can probably remove it.

DyLP:

  • Master filters triggers to only src and test changes

    • Proposal: Remove filters

      OK, but I guess we should let Lou Hafer know, he probably did that and still dabbles in CyLP. It may actually be the right thing to do, I'm not sure.

SYMPHONY:

  • "WS": Stable also builds from branch called WS (added two years ago)

    • Proposal: Remove WS branch building

      WS was something important to build from until fairly recently, but yes, now it can be removed I think.

  • "mysql": Stable has "--disable-mysql " that is not in master, "For now, add --disable-mysql to fix problem with Glpk"

    • Proposal: Remove disable-mysql from stable.

      Yes, I struggled quite a bit with this. The build was failing because of Glpk's optional use of mysql for some arcane reason that I never quite worked out. I didn't quite see why this wasn't an issue for other projects. It was a quick fix at the time, we can see what happens if we remove it, but I guess the build might just start failing again.

jhmgoossens commented 6 months ago

Apologies, I got fed up now that both the linux (macos-13 issue) and windows (this here) were Failing, so I committed linux-ci and windows-ci changes starting with CoinUtils master and stable.

Thanks for reviewing so thoroughly, and what an impressive memory!

We only do this in master and not in stables because we previously did not have the compiler directives needed to expose only the parts of the interface we want, so the result is that everything is hidden in stable. Nothing would build correctly with this option.

I dont understand exactly. The "visibility=hidden" is done mostly in master branches. Anyway, if I understand correctly that this setting affects interfaces, then we should arguable not change this in the current stable, since it could break interfaces--Leave such a change to the next stable version.

Proposal: Good point, but can be resolved by removing the head_ref from the asset name altogether. This should be used in all workflows.

I don't know why this was never an issue for other projects. If you look at the file names for the releases of those projects, they already have / replaced with ., so the action itself must have changed and we just haven't made any releases since that change (other than MibS). I do recall now, though, that when I was poking at this to see what happened, I happened to see that actions/upload-release-asset is now deprecated and there are some better options. This is one of the things that I thought should be changed and this may fix the issue as well. I'm not sure what you mean by not having head_ref as part of the filename, though. You mean just the version number and not releases/? Then I guess you still need to filter to get the version number. The version number should be there I think, right?

No, this is not about the upload-release-asset (which indeed seems to work), but about upload-artifact for pull requests. See https://github.com/coin-or/CoinUtils/actions/runs/8271068099 as an example. These PR artifacts dont actually need the branch name (head_ref) in their name, because within the context of the PR, it is clear from which branch this PR was made.

that actions/upload-release-asset is now deprecated

Yes, I havent gotten around to that part of the review yet, though that is somewhat easier than harmonizing all those changes..

tkralphs commented 6 months ago

Hmm not sure why I did the quote indentation backwards (indenting my replies more than your originals).

  • "visibility=hidden": Many matrix entries use "flags: [ADD_CXXFLAGS=-fvisibility=hidden]", but often the stable workflow doesnt contain this.

Proposal: Add flags: [ADD_CXXFLAGS=-fvisibility=hidden] to master and stable.

We only do this in master and not in stables because we previously did not have the compiler directives needed to expose only the parts of the interface we want, so the result is that everything is hidden in stable. Nothing would build correctly with this option.

I don't understand exactly. The "visibility=hidden" is done mostly in master branches. Anyway, if I understand correctly that this setting affects interfaces, then we should arguable not change this in the current stable, since it could break interfaces--Leave such a change to the next stable version.

It is possible to expose only some of the functions as an API wile hiding others. This is done in Windows using __declspec(dllexport) and in Linux using __attribute__((__visibility__("default"))). In the current stables, this is not done at all and is the reason we could not really build DLLs because in Windows, all functions are hidden by default. Fortunately, it is the opposite with gcc and everything is exposed by default, unless you specify visibility=hidden. Since we don't handle this at all in the stable branch, We would not be able to build shared libraries at all with visibility=hidden because all functions would be hidden and nothing could be linked.

In the new build system used by master, we fixed all of this and now handle it by defining a symbol that resolves differently on different platforms, such as here: https://github.com/coin-or/Cbc/blob/46d35963c7fe46830ffb59885c815e39051e58e5/src/CbcConfig.h#L36-L41 It s used as here: https://github.com/coin-or/Cbc/blob/46d35963c7fe46830ffb59885c815e39051e58e5/src/CbcSolver.hpp#L63C7-L63C17

The visibility=hidden flag is used basically to ensure that we are not forgetting to export something that should be exported. Otherwise, it is not needed. Honestly, I don't know how useful this all is, since the person building would need to use that flag for any of it to have effect, but perhaps people do use these flags in production.

tkralphs commented 6 months ago

No, this is not about the upload-release-asset (which indeed seems to work), but about upload-artifact for pull requests. See https://github.com/coin-or/CoinUtils/actions/runs/8271068099 as an example. These PR artifacts dont actually need the branch name (head_ref) in their name, because within the context of the PR, it is clear from which branch this PR was made.

Ah, so this only affects artifacts attached to builds from PRs? I suppose you are right. This probably came up in MibS then because I think there was a PR in which a stable was being merged back into master. OK, so this should probably not be an issue.

jhmgoossens commented 6 months ago

Starting with CoinUtils I updated now also the deprecated actions (except upload-release-asset) for the windows-ci and linux-ci.

About the name used in action\upload-artifact for PR assets: I was hoping that the updated version of upload-artifact@v4 for PRs might resolve the asset name issue ('head_ref'), but unfortunately not. So, as proposed, I simply removed the branch name ('head_ref') from the PR artifact name, et voila.

tkralphs commented 6 months ago

Great! So what about the deprecated upload-release-asset? I think there was a new action that I found that had some improved capabilities and was maintained.

jhmgoossens commented 6 months ago

The github page of upload-release-asset mentions softprops/release-gh-action. However, I believe that one does more than upload the asset to an existing release—it also creates the release. How exactly do our current releases get created? I didn’t want to mess up the Releases while getting this to work, so left it for later.

jhmgoossens commented 6 months ago

I just realized that I forgot to compare also the Windows-ci between master and stable. General:

tkralphs commented 6 months ago

@jhmgoossens No pressure at all, but are you planning to continue with other projects?

jhmgoossens commented 6 months ago

@tkralphs Yes! I'm continuing with CHiPPS-ALPS/BiCePS/BLIS and then DyLP, MibS and SYMPHONY. I forked these and will be making simple PRs like the PRs for master and stable waiting in https://github.com/coin-or/CHiPPS-ALPS/pulls. I don't have permissions on these projects--and that is good. Shall I put a comment for you whenever I create a PR?

These projects don't have recent Visual Studio solutions that can be used in the "MSVS" job in the windows-ci actions. Therefore, I have included all the same action steps, but just not the matrix entry.

Note also the two open questions above: How we do Releases for "upload-release-asset", and how to deal with "enable-msvc" differences?

tkralphs commented 6 months ago

Shall I put a comment for you whenever I create a PR?

I should get notified, but sure, doesn't hurt!

How we do Releases for "upload-release-asset"

We have a release.yml which lives with the other files you've been modifying already. Looks like it uses the create-release action.

how to deal with "enable-msvc"

The differences are probably not really by design and I've never really known exactly what would be useful for others. But MD seems like the right default. What is done if nothing is specified is a function of the BuildTools, not coinbrew. coinbrew passes configure arguments straight through. The behavior is completely different for the master branch of BuildTools and the stable branch.

Although I have tried to figure out what exactly these flags mean many times over the years, I have failed every time. But we should probably do some experimenting. In the end, it's also probably a matter of just trying something and seeing if anyone complains.

jhmgoossens commented 5 months ago

Thanks for the info!

how to deal with "enable-msvc" it's also probably a matter of just trying something and seeing if anyone complains.

I'll give that a try.. I compared the msvc16 runs on GitHub of Clp master with the updated "enable-msvc" to the old "enable-msvc=MD". The msvc16 binaries from "enable-msvc" are really MSVC binaries. The msvc16 binaries from "enable-msvc=MD" are not MSVC binaries but require libgcc and libstd++, and seem identical to the x86_64-w64-mingw64 binaries. Or maybe my expectations are wrong? I'd say "enable-msvc=MD" is just wrong.

How we do Releases for "upload-release-asset" We have a release.yml which lives with the other files you've been modifying already. Looks like it uses the create-release action.

Do I understand correctly that in order to create a Release, you manually create a Tag? This then automatically triggers a run of the Release action (on: push: tag: ), which creates a Release. Next, the windows-ci and linux-ci are triggered upon creation of a Release (on: push: release: ) and they finally both upload their binaries to that one Release. In this way, we get one Release with assets from both linux-ci and windows-ci.

Both actions/create-release and actions/upload-release-asset are no longer maintained and/or use outdated node.js. These two mention several alternative actions, but only the following two are actually still maintained:

The ncipollo/release-action seems like the way forward, both for release.yml and the ci.ymls. I'll try to test this by adding Tags in a fork, to keep the repos clean of test releases.

But first I'll complete the first harmonization.

Let me know if you have any comments!

tkralphs commented 5 months ago

Do I understand correctly that in order to create a Release, you manually create a Tag?

Ah, OK, I didn't think about the fact that the tagging itself could be part of the release process. Yes, I think you understand the flow correctly. Currently, the tags are not created manually per se, but by a script that does a lot of different things related to creating a release. The current version is here (there is a similar but different one for the master branches). It automatically determines the release version number, changes it in the appropriate places, re-runs the autotools, runs the unit test, etc. For an example, look at the last release of Cbc here.

Note that it says the commit doesn't belong to any branch. The last step of the script is to create the tag, which is done in a separate commit outside the stable branch so as not to pollute the stable branch with the change of version number, etc., which would need to be immediately changed back. So there is probably no really easy way to have the tag created as part of an action, since that probably assumes the release is the tip of some branch, which it isn't for us.

ncipollo/release-action looks pretty good, but thinking about it now, depending on actions that are not maintained by Github might not be a great idea. All in all, it probably makes sense to fork all the actions we use so we can modify to suit and control updates to the fork when new versions are released upstream as needed, as described here. I don't really want to create a new organization just for this, as the author suggests, but we could put them into coin-or-tools. What do you think?

jhmgoossens commented 5 months ago

ncipollo/release-action looks pretty good, but thinking about it now, depending on actions that are not maintained by Github might not be a great idea. All in all, it probably makes sense to fork all the actions we use so we can modify to suit and control updates to the fork when new versions are released upstream as needed, as described here. I don't really want to create a new organization just for this, as the author suggests, but we could put them into coin-or-tools. What do you think?

Unfortunately, it seems GitHub is maintaining fewer and fewer actions itself, relying instead on the community. I'm not really in favor of forking actions. If we're not actually going to be reviewing their code, then forking just means more work for nothing (if we keep our fork updated), or longer time to get fixes to issues (if we don't). However, I agree we should not blindly follow the latest versions of any dependency. Instead, how about

  1. Configuring GitHub Dependabot for security alerts for actions
  2. Pinning an exact version of the non-GitHub dependency (@v1.2.3)

Honestly, I'd go with Dependabot and then pinning only the major release of the dependency (@v1), but maybe I'm naive.

[Edit] I believe Dependabot is already on for the whole organization, right? I wanted to see what happens when a vulnerable action is included, so in a fork I used tj-actions\changed-files@40 and indeed, Dependabot alerted me about this action and created a PR because I turned on "Dependabot security updates".

tkralphs commented 5 months ago

I'm not really in favor of forking actions.

OK, I get your point. I originally thought of this because I thought we might be forced to build some custom actions to get what we want, starting from existing ones. If we end up wanting tweaks, but also want to get updates, then forking makes sense. But this probably isn't necessary after all.

Honestly, I'd go with Dependabot and then pinning only the major release of the dependency (@v1), but maybe I'm naive.

OK, sure, there is a small risk that a new patch release could introduce something malicious, but I guess we can live with that.

I believe Dependabot is already on for the whole organization, right?

It's not that clear to me. I did receive some alerts over time about projects I don't directly manage, but I've never seen an automated PR like you demonstrated, but I don't think we have that many dependencies explicitly visible to Github. Believe it or not, I cannot see an easy way to know whether security features are enabled organization-wide. I hit the "Enable All" button for as many features as I could, but the button is still available to hit again, which is weird. Anyway, as far as I know, yes, now, it is enabled for all projects.

tkralphs commented 5 months ago

I just became aware that there are runners for MacOS on Apple Silicon, we could consider adding those. Basically, it should be just be a matter of adding

          - os: macos-14
            arch: arm64
jhmgoossens commented 5 months ago

OK, do you propose to add as third macos matrix entry after the macos-13 clang and gcc entries, or replace the two macos-13 with this macos-14 with that arch?

tkralphs commented 5 months ago

I was proposing to add as a third entry, since I think we still want the Intel-based builds for now.

jhmgoossens commented 5 months ago

So macos-14 builds fine. Good point, to keep all three (unlike in the above PR). I'll leave the other two on macos-13, and merely add macos-14 arm64 gcc.

Note I created an Issue for the coin-or-tools platform-analysis-tools for wrong macos version string, which seems to have been around for a while (macos-12 artifacts are called "macos106", e.g.)

jhmgoossens commented 5 months ago

@tkralphs Thanks for fixing platform-analysis-tools.

With the exception of the create-release and upload-release-assets outdated issue, all changes to windows-ci and linux-ci are now pushed or will be pushed shortly:

These changes are committed to Cbc, Clp, Cgl, CoinUtils, Osi master and latest stables.

TODO: Now creating PRs for master and stables for CHIPPS-ALPS, etc.

Some notes:

  1. For simplicity I simply made separate PRs for masters and latest stables--not cherry picks. The downside is that therefore the master seems at least one commit behind the latest stable (which could be confusing), because these are separate commits. Only for Cbc I cherry picked the master commit into latest.
  2. In the commit message I made a typo "Add macos-13 .." should be "Add macos-14.
jhmgoossens commented 5 months ago

These projects don't have recent Visual Studio solutions that can be used in the "MSVS" job in the windows-ci actions. Therefore, I have included all the same action steps, but just not the matrix entry.

@tkralphs How shall I do the Windows-ci updates for the other projects, notably for the MSVS (Visual Studio) build steps? I realize for MSVS these add a lot of steps that are not used in these projects. Therefore, I can either

  1. include the MSVS steps but not the matrix entry advantage: the windows-ci are nearly identical, disadvantage: many unused steps
  2. leave out the MSVS steps altogether advantage: no unused steps, disadvantage: different windows-ci
tkralphs commented 5 months ago

Hmm, I thought I responded to this, but I guess I didn't! I needed to fix up the MibS CI to make a release and for that one, I left out the MSVS steps altogether. Having two "templates", one for projects with VS solutions and one for projects without doesn't seem too bad. But it occurs to me that the real solution is probably to have a fourth yaml file that does the MSVS and just isn't included in the repos where we don't need it. Is there a down side to that, aside from the fact that there is some work involved in modifying lots of files one more time?

jhmgoossens commented 5 months ago

Thanks for your feedback and good point!

have a fourth yaml file that does the MSVS and just isn't included in the repos where we don't need it. Is there a down side to that, aside from the fact that there is some work involved in modifying lots of files one more time?

But the current Windows yaml indeed has "if matrix.arch == 'msvs'" in about half the steps. And if other projects want to start building using MSVS, then we can just copy them the fourth yaml.

First, for the remaining projects I'll leave out the MSVS steps altogether like you did. After that, I'll review the Cbc and dependencies to move the MSVS steps to the fourth "windows-msvs-ci.yml". Because these use some secrets, I may need your help with that.

jhmgoossens commented 5 months ago

I created all PRs for master and stable for CHiPPS-ALPS, CHiPPS-BiCePS, CHiPPS-BLIS, MibS, Symphony and DyLP. These windows-ci are without all the 'MSVS' steps.

All the CHiPPS-* projects PRs checks pass and are awaiting your review.

The PR for MibS stable (which you had already updated) includes the 'remaining' minor updates to get also MibS stable aligned to the other yamls.

Unfortunately, the DyLP master PR but also the Symphony master PR and consequently also MibS master PR have issues in the Linux builds. I had a look, but I cant see how to fix these issues. Any suggestions?

tkralphs commented 5 months ago

Hmm, that is weird. The SYMPHONY issues is that clang indeed does not support -fopenmp, but configure is not supposed to add that flag when building with clang. I'll have to check what's going on there. Should be an easy fix.

tkralphs commented 5 months ago

The problem seems to be that you removed the CXX=clang++. Without it, the compiler is detected as g++, which is really a wrapper around clang++ on MacOS. This only causes a problem with SYMPHOY because it decides whether to add -fopenmp based on whether the compiler is g++ or not.

It's probably best to add it back everywhere, since otherwise, configure has no way of knowing whether it's using clang or gcc and this could matter at some point, though it doesn't seem to now. Having said that, I know it could be a huge pain to do that so we can always fix this in the next round.

tkralphs commented 5 months ago

The DyLP error seems to be a legit issue that needs to be addressed by the author, so we can just leave that one sit for now.

jhmgoossens commented 5 months ago

you removed the CXX=clang++.

Sorry about that!

Could it be that originally only SYMPHONY and MibS use clang with "CXX=clang++"? I looked at Cbc, CHiPPS-ALPS, BiCePS, BLIS and DyLP, and they all originally didnt have CXX=clang++, I believe. I should have noticed this difference in SYMPHONY and MibS and left it in, but didnt :-(

It's probably best to add it back everywhere

Do you mean that CXX=clang++ should be in the baseline yml used for linux-ci for all projects? I dont mind updating, but it seems it is more an exception for SYMPHONY and MibS?

tkralphs commented 5 months ago

Could it be that originally only SYMPHONY and MibS use clang with "CXX=clang++"?

Yes, that is entirely possible.

Do you mean that CXX=clang++ should be in the baseline yml used for linux-ci for all projects? I don't mind updating, but it seems it is more an exception for SYMPHONY and MibS?

It's not so much that it's an exception for SYMPHONY and MibS, it's more that we are getting lucky to a certain extent that this is working with most projects. I could imagine any number of things going wrong at some point in the future due to the configure script thinking that it is working with g++ when it is actually working with clang++. The worst of these would be that the real g++ is actually installed on the build machine and we end up using that when we intended to use clang++, so clang is not really being tested when we thought it was.

It's not a big deal for this to wait until our next batch change for some other reason, but in the long run, I guess it should be done.

jhmgoossens commented 5 months ago

@tkralphs Thanks for explaining why we need to add CXX=clang++! I created #28 to track that.

Regarding splitting Windows builds for Visual Studio ('msvs') to a separate yaml: I implemented this in master of CoinUtils. The remaining windows-ci.yml is just like the one you had already updated manually. See also https://github.com/coin-or/CoinUtils/commit/014be1f1724c074401d9d9c27bcce35baa9dca45 for all changes.

I'll update the masters and stables of Cbc, Clp, Cgl, CoinUtils and Osi with this split msvs.

Once that is completed, I propose to close this issue. Let me know if you have any comments.

tkralphs commented 5 months ago

Sounds good, thanks so much for your help on this and for pushing it ahead!

jhmgoossens commented 5 months ago

The split of windows ci is completed for masters and stables of Cbc, Cgl, Clp, Osi, and CoinUtils.

All the PRs are still awaiting approval for masters and stables of linux and windows ci (without the msvs steps) for CHiPPS-*,MibS, SYMPHONY and DyLP, outside of my control.

Overall, this closes this issue.

tkralphs commented 5 months ago

All PRs have been merged. I guess we should archive the "standard" version of these files here: https://github.com/coin-or/coinbrew/tree/master/.ci What is there now is old, from before we moved to Github Actions.