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

kernel: remove SyOriginalArgv & SyOriginalArgc #5887

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

fingolfin
Copy link
Member

These kept references to the argv array around. This is potentially problematic for consumers of libgap API, specifically when calling GAP_Initialize it is not obvious that one has to keep the argv passed to it alive until after GAP completed its startup.

While we could amend the documentation for GAP_Initialize (which unfortunately currently does exist at all) to mention this, it seems better to obviate the need for it.

This patch is a first step towards this. However, several pointers to argv members remain, at least these:

  • SyCompileOutput
  • SyCompileInput
  • SyCompileName
  • SyCompileMagic1
  • SyRestoring

@fingolfin fingolfin added topic: kernel release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Jan 3, 2025
Copy link
Contributor

@ChrisJefferson ChrisJefferson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clearly moving things in a good direction

@fingolfin
Copy link
Member Author

The Cross compiler for s390x CI job consistently fails, across multiple runs by now. So this doesn't look like a pure fluke anymore.

But I still can't figure out why it would do that. What change in this PR could even be responsible?!?

@ChrisJefferson if you have any idea at all...

@fingolfin
Copy link
Member Author

It hangs in test suite testworkspace, which may give a clue...

@fingolfin
Copy link
Member Author

OK, dev/ci.sh testworkspace also hangs on my mac! Now I have to wonder: why do all other CI tests pass?

These kept references to the `argv` array around. This is
potentially problematic for consumers of libgap API, specifically
when calling `GAP_Initialize` it is not obvious that one has to
keep the `argv` passed to it alive until after GAP completed its
startup.

While we could amend the documentation for `GAP_Initialize`
(which unfortunately currently does exist at all) to mention this,
it seems better to obviate the need for it.

This patch is a first step towards this. However, several pointers
to `argv` members remain, at least these:
- `SyCompileOutput`
- `SyCompileInput`
- `SyCompileName`
- `SyCompileMagic1`
- `SyRestoring`
@fingolfin
Copy link
Member Author

So the problem was that when restoring a workspace, KernelArgs was reset to its value from when the workspace was created, not the current list of command line arguments.

The true mystery thus is why testworkspace still passed on Linux. Weird. In any case, this was a close one, only our cross compile test suite running on OpenBSD caught it. I now made sure we also run testworkspace on macOS where the issue also appears.

@fingolfin fingolfin enabled auto-merge (squash) January 6, 2025 10:57
@limakzi
Copy link

limakzi commented Jan 6, 2025

@fingolfin

  • Ad Continuous Integration.
    I suppose we could make builds on macOS as well. [1]

  • Ad code. LGTM. 🎉

@fingolfin
Copy link
Member Author

@limakzi we already run CI tests on macOS runners, just fewer than on Linux because they are bottlenecked (we can have 40 concurrent Linux jobs but only 5 concurrent macOS jobs, IIRC)

@fingolfin
Copy link
Member Author

@limakzi thanks for the review!

@fingolfin fingolfin merged commit d577ec1 into gap-system:master Jan 6, 2025
33 checks passed
@fingolfin fingolfin deleted the mh/SyOriginalArgv branch January 6, 2025 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants