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

Add VS2022 and its windows-ci to stable 2.11 #214

Merged
merged 10 commits into from
May 17, 2023

Conversation

jhmgoossens
Copy link
Contributor

While master branch has had VS2022 projects for some time, these were not in stable/2.11 since that was created before.
Added also windows-ci build msvs for VS2022 like master.
Included configall_system for MSC as in master.

PR builds failed due to package names (the "/" in remote branch name stable/2.11-dev made the original package name not valid). Fixed Windows ci builds, not in Linux ci (see #212 ).

The aim is to get Cbc stable/2.10 and all (stable) dependencies to have VS2022 projects where currently none do.
CoinUtils stable/2.11 is step 1.

@jhmgoossens jhmgoossens requested a review from tkralphs May 15, 2023 09:38
@jhmgoossens jhmgoossens self-assigned this May 15, 2023
@tkralphs
Copy link
Member

Sorry to be a pain, but I guess that the Windows safe naming fix is also now not needed. Ideally, Windows and Linux would handle artifact naming in the same way, since this is independent of build platform. I guess that the first four commits here are the ones really needed. Maybe you would reset head back to 51f4346 and force push?

@jhmgoossens
Copy link
Contributor Author

jhmgoossens commented May 15, 2023

I see what you mean. I’ll try to change the PR again or create a third PR if necessary.
I offer my help with future ci yaml maintenance of all projects, if needed.

However, three points to keep in mind for the future:

  • We now have an undocumented requirement that remote branch names for PRs must not have / in their names, even though these are perfectly fine branch names and even the target branch has a / in its name.

  • the artifact of upload-artifact for PRs live inside the PR only. And since the PR already stores the remote branch name, I would even suggest that for PR builds we stop putting the remote branch name in the PR artifact name thus preventing the problem.

  • Lastly, the upload-release-asset action (with the magic renaming) is not maintained and proposed to be replaced by softprops/action-gh-release. I don’t know how that one handles the magic renaming of / to . Whenever we find time to change action to action-go-release then we can reconsider the asset naming.

@jhmgoossens
Copy link
Contributor Author

I took the easy way out and simply added a commit to revert the windows-ci changes. I propose to squash and merge changes.

@tkralphs
Copy link
Member

OK, sure, I can squash and merge. Your points are valid and I agree that we should make a better fix eventually. Not including the remote branch name in the artifact name makes sense. If we either had a significant number of PRs coming in so that this could actually be an issue or more developer bandwidth, I would push something out more quickly.

I do periodically push out new YAML files to all project when there are enough fixes to justify the effort, I just want to do this purposefully. Let's open an issue to keep track of all the things that should be fixed and then when that list has enough things on it, we can push out new YAML files to all projects. A logical place to open the issue would be in https://github.com/coin-or/Coin-OR-OptimizationSuite. And sure, I would appreciate the help with this!

@jhmgoossens
Copy link
Contributor Author

Great, thanks. I created the issues.
Apologies, I didn’t appreciate the very long list of projects in the Suite, until just now.

I’ll now proceed with (just) the Cbc dependencies and their latest stables.
I will try to work on VS 2022 for more projects in the Suite, as Issue 10 requests.

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

Successfully merging this pull request may close these issues.

2 participants