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

Github CI/CD updates #450

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

bscottm
Copy link
Contributor

@bscottm bscottm commented Mar 4, 2025

  • Nuke and reinstall HomeBrew for macOS builds. Gets around the whole problem of stale links and conflicting packages installed in the GitHub macOS images.

  • macOS 12 and Ubuntu 20.04 officially deprecated. Ubuntu 20.04 will be removed from GitHub's runner images on 01 APR 2025.

  • .travis/deps.sh: Add mingw32 as a dependency target, enable MinGW64 32-bit builds.

  • Work around LTO bug on Ubuntu and macOS compilers, where LTO appears to miscalculate the number of bytes stored when using the default byte swapping 'for' loop. This is a workaround that uses compiler swapping intrinsics for well known sizes that appeases LTO. The alternative is to wait for a compiler fix, which is infeasible.

- Nuke and reinstall HomeBrew for macOS builds. Gets around the whole
  problem of stale links and conflicting packages installed in the GitHub
  macOS images.

- macOS 12 and Ubuntu 20.04 officially deprecated. Ubuntu 20.04 will be
  removed from GitHub's runner images on 01 APR 2025.

- .travis/deps.sh: Add mingw32 as a dependency target, enable MinGW64
  32-bit builds.

- Work around LTO bug on Ubuntu and macOS compilers, where LTO appears
  to miscalculate the number of bytes stored when using the default byte
  swapping 'for' loop. This is a workaround that uses compiler swapping
  intrinsics for well known sizes that appeases LTO. The alternative is
  to wait for a compiler fix, which is infeasible.
@bscottm
Copy link
Contributor Author

bscottm commented Mar 4, 2025

SEL32 choked on its tests. @pkoning2: Please re-run.

@bscottm
Copy link
Contributor Author

bscottm commented Mar 17, 2025

@pkoning2: Really need this to merge so that regular CI/CD builds will have some chance at success, barring SEL32 test failures.

@markpizz
Copy link
Contributor

Why did you make BSWAP activities (in sim_fio.c) part of this commit? How is that related to CI/CD?

@pkoning2
Copy link
Member

If LTO is generating bad code, why would we use it? It feels like it's an experimental feature, or at least in the earlier releases of it. If it's not ready for prime time, let's not create weird workaround, instead just avoid it.

@Rhialto
Copy link
Contributor

Rhialto commented Mar 17, 2025

If I remember right, earlier versions of the simh package in pkgsrc went to some lengths to avoid LTO. That seemed a bit silly since it didn't look like simh itself used it, but looking at its Makefile more recently it looked like it can be used. But yes, if it is problematic, just avoid it.

@bscottm
Copy link
Contributor Author

bscottm commented Mar 17, 2025

Why did you make BSWAP activities (in sim_fio.c) part of this commit? How is that related to CI/CD?

@markpizz: Have a look at the makefile-s output on recent builds and pay particular attention to the errors produced by the LTO pass. No. Really. Study it.

The BSWAP activities are there so that YOU can still keep LTO turned on in the makefile and successfully get through the CI/CD on GitHub. It would appear that LTO interprets the code in sim_fio.c as it attempts to inline the swap, gets things wrong, and generates a fatal error. The BSWAPs are there to give LTO a SUPER BIG HINT for well-known sizes.

@bscottm
Copy link
Contributor Author

bscottm commented Mar 17, 2025

If LTO is generating bad code, why would we use it? It feels like it's an experimental feature, or at least in the earlier releases of it. If it's not ready for prime time, let's not create weird workaround, instead just avoid it.

@pkoning2: @markpizz added LTO to the makefile two years ago because LTO produced a useful warning. See 2b9e2eb, although no explanation is given in the commit message.

I'd much prefer to drive -Wall warnings down to a minimum (preferably, to zero) so that it's easier to identify issues. But that's just a personal preference.

The CMake builds don't do LTO. I came to the conclusion (contusion?) that the juice wasn't worth the squeeze.

@markpizz
Copy link
Contributor

The BSWAP activities are there so that YOU can still keep LTO turned on in the makefile and successfully get through the CI/CD on GitHub.

OK, BUT I suggest that the BSWAP changes/optimizations deserve it's own commit and the CI/CD stuff another.

@bscottm
Copy link
Contributor Author

bscottm commented Mar 17, 2025

The BSWAP activities are there so that YOU can still keep LTO turned on in the makefile and successfully get through the CI/CD on GitHub.

OK, BUT I suggest that the BSWAP changes/optimizations deserve it's own commit and the CI/CD stuff another.

@markpizz: It's really hard to complete a CI/CD build without it. Part and parcel.

@markpizz
Copy link
Contributor

The BSWAP activities are there so that YOU can still keep LTO turned on in the makefile and successfully get through the CI/CD on GitHub.

OK, BUT I suggest that the BSWAP changes/optimizations deserve it's own commit and the CI/CD stuff another.

@markpizz: It's really hard to complete a CI/CD build without it. Part and parcel.

I am not disputing your claim. I am suggesting that THIS PR should have 2 commits. The first being the BSWAP changes and the second the CI/CD stuff.

@bscottm
Copy link
Contributor Author

bscottm commented Mar 17, 2025

If I remember right, earlier versions of the simh package in pkgsrc went to some lengths to avoid LTO. That seemed a bit silly since it didn't look like simh itself used it, but looking at its Makefile more recently it looked like it can be used. But yes, if it is problematic, just avoid it.

LTO opportunistically inlines and merges code through call graph analysis in the object files when a simulator is linked. SIMH doesn't actually use it "directly" as such.

@bscottm
Copy link
Contributor Author

bscottm commented Mar 17, 2025

I am not disputing your claim. I am suggesting that THIS PR should have 2 commits. The first being the BSWAP changes and the second the CI/CD stuff.

The protocol has been single commit per PR. You're suggesting I break protocol?

@markpizz
Copy link
Contributor

I' never had such a protocol.

I did have a general rule that all the possibly many commits that may have happened in the course of development be squashed to be tightly related to a single purpose that a PR addresses. That may involve separate commits depending on the logic affecting the relevant modules. In addition, general, code changes would be separate from changes to build facilities, with the commit(s) necessary code changes coming BEFORE the changes to build activities that needed them.

@pkoning2
Copy link
Member

If I remember right, earlier versions of the simh package in pkgsrc went to some lengths to avoid LTO. That seemed a bit silly since it didn't look like simh itself used it, but looking at its Makefile more recently it looked like it can be used. But yes, if it is problematic, just avoid it.

LTO opportunistically inlines and merges code through call graph analysis in the object files when a simulator is linked. SIMH doesn't actually use it "directly" as such.

Yes, I know what LTO does. It's pretty new technology, and its usefulness for SIMH is questionable to put it mildly. If it is known to generate bad code, I don't want it to be the default, and in the interest of safety I would prefer not to have it used at all if it still so immature.

@bscottm
Copy link
Contributor Author

bscottm commented Mar 18, 2025

I' never had such a protocol.

Seems to be the protocol to which I've been asked previously to adhere.

I did have a general rule that all the possibly many commits that may have happened in the course of development be squashed to be tightly related to a single purpose that a PR addresses. That may involve separate commits depending on the logic affecting the relevant modules. In addition, general, code changes would be separate from changes to build facilities, with the commit(s) necessary code changes coming BEFORE the changes to build activities that needed them.

You can cherry pick commits and apply them them to individual files. git cherry-pick <hash> -- sim_fio.c is supposed to do this for you. Then commit the result.

@bscottm
Copy link
Contributor Author

bscottm commented Mar 18, 2025

Yes, I know what LTO does. It's pretty new technology, and its usefulness for SIMH is questionable to put it mildly. If it is known to generate bad code, I don't want it to be the default, and in the interest of safety I would prefer not to have it used at all if it still so immature.

It's a recent compiler update. LTO has been in the makefile for around 2 years without reported issues.

Sorry -- the way I interpreted your previous reply was that "use" referred to an API as opposed to in reference to compiling and optimizing.

@markpizz
Copy link
Contributor

I never had such a protocol.

Seems to be the protocol to which I've been asked previously to adhere.

Admittedly, many of your PRs have one commit, however a quick scan of closed PRs from you shows at least #430, #398 and #316 having multiple commits. Clearly the things in this PR have 2 logical things going on with the second one dependent on the first.

Feel free to make and follow whatever arbitrary protocol you want.

@bscottm
Copy link
Contributor Author

bscottm commented Mar 18, 2025

Feel free to make and follow whatever arbitrary protocol you want.

@markpizz: I adhere to the guidance and advice given to me by Paul and the steering group.

@LegalizeAdulthood
Copy link

I adhere to the guidance and advice given to me by Paul and the steering group.

IMO, this is the right way to go. If you wanted to follow that other guy's advice, you wouldn't be making pull requests against OpenSIMH.

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.

5 participants