Skip to content

Conversation

@trinistr
Copy link
Contributor

@trinistr trinistr commented Nov 20, 2025

Continuing from #1297

  • Added a spec for .map. It has a lot of edge cases.
  • Added specs for .for and .string. Not much to say about these, they behave according to documentation and don't have complicated arguments.
  • Added more cases for #initialize (.new).
  • Removed most cases for predicate methods, leaving just a few representative examples, as flags are checked in constructor specs.

More weird stuff:

@trinistr trinistr force-pushed the add-io-buffer-constructors-specs branch 5 times, most recently from fb299b4 to 60c35d9 Compare November 20, 2025 18:36
@eregon
Copy link
Member

eregon commented Nov 20, 2025

@ioquatix Could you review this maybe?

@trinistr
Copy link
Contributor Author

@ioquatix Could you review this maybe?

This is still a very early draft, but that offset parameter made me create an issue and a PR to ruby directly, so, yeah 🥹 Reviewing the PR with fixes for the method is probably more useful currently.

@trinistr trinistr force-pushed the add-io-buffer-constructors-specs branch 2 times, most recently from c38d813 to 903d51d Compare November 29, 2025 08:46
@ioquatix ioquatix force-pushed the add-io-buffer-constructors-specs branch from 903d51d to fc60523 Compare December 6, 2025 21:09
@trinistr trinistr force-pushed the add-io-buffer-constructors-specs branch 4 times, most recently from 9ebc319 to bc2f82c Compare December 17, 2025 14:26
@trinistr trinistr marked this pull request as ready for review December 17, 2025 14:49
@eregon
Copy link
Member

eregon commented Jan 4, 2026

LGTM in general

@trinistr trinistr force-pushed the add-io-buffer-constructors-specs branch 3 times, most recently from 8ffe059 to 395273f Compare January 5, 2026 20:24
@trinistr trinistr force-pushed the add-io-buffer-constructors-specs branch from 395273f to 012876d Compare January 6, 2026 11:43
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Just needs a couple tweaks

.map has a lot of edge cases that are practically undefined behavior and may cause syscall error or crashes.
Also simplify specs for predicate methods, as flags are mostly set on buffer creation and tested accordingly.
@trinistr trinistr force-pushed the add-io-buffer-constructors-specs branch from 012876d to 49b4b7f Compare January 7, 2026 15:04
@eregon eregon merged commit 74bbc74 into ruby:master Jan 7, 2026
16 checks passed
@trinistr trinistr deleted the add-io-buffer-constructors-specs branch January 8, 2026 02:21
@eregon
Copy link
Member

eregon commented Jan 29, 2026

FYI there were a bunch of failures on https://rubyci.org/ caused by these specs, e.g.
https://rubyci.s3.amazonaws.com/debian/ruby-master/log/20260129T063003Z.fail.html.gz
http://rubyci.s3.amazonaws.com/openbsd-current/ruby-master/log/20260129T063005Z.fail.html.gz
http://rubyci.s3.amazonaws.com/freebsd13/ruby-master/log/20260129T083001Z.fail.html.gz
http://rubyci.s3.amazonaws.com/freebsd14/ruby-master/log/20260129T083002Z.fail.html.gz
http://rubyci.s3.amazonaws.com/freebsd15-arm/ruby-master/log/20260129T043002Z.fail.html.gz
http://rubyci.s3.amazonaws.com/s390x/ruby-master/log/20260129T060003Z.fail.html.gz

@nobu fixed them in ruby/ruby@cd26647...e48ed4b, thank you and sorry. I think the remaining failures are just waiting for new CI runs.

I'm guessing the reasons for the CI in repo not catching that is:

  • I think some of RubyCI don't have a locale set, or equivalent to LC_ALL=C/LANG=C. We should add a CI job here to catch this earlier: Add a CI job to run specs with LC_ALL=C #1336
  • mmap() seems quite inconsistent how it behaves on various platforms. That's how it is and it sucks (I've had the same experience or even worse with clock_getres(), that's basically broken in some way one very platform), but I think the only thing we can do is be careful to not be overly specific about e.g. system errors or behavior for edge cases. To the extent possible I think would make sense for Ruby to shield us from this variability by having reliable checks e.g. for size 0, incorrect offset, etc, cc @ioquatix.

eregon added a commit that referenced this pull request Jan 29, 2026
* Which simulates having no locale set and using the "default" locale.
* To catch failures earlier rather than in https://rubyci.org/
* See #1305 (comment)
eregon added a commit that referenced this pull request Jan 29, 2026
* Which simulates having no locale set and using the "default" locale.
* To catch failures earlier rather than in https://rubyci.org/
* See #1305 (comment)
eregon added a commit that referenced this pull request Jan 29, 2026
* Which simulates having no locale set and using the "default" locale.
* To catch failures earlier rather than in https://rubyci.org/
* See #1305 (comment)
@trinistr
Copy link
Contributor Author

Oof, didn't expect that there would be this many problems, sorry everyone 🙇

To the extent possible I think would make sense for Ruby to shield us from this variability by having reliable checks e.g. for size 0, incorrect offset, etc

Almost all UB cases are already checked in 4.0. As far as I'm aware, the only major problem left is unreliability of allowed values for offset (https://bugs.ruby-lang.org/issues/21700).
But it looks like I was too optimistic about mmap raising some error in these cases.

http://rubyci.s3.amazonaws.com/openbsd-current/ruby-master/log/20260129T063005Z.fail.html.gz

I see failures on OpenBSD related to sharing mmaped memory across processes, curious. Don't see anything in documentation for why it wouldn't be shared. @eregon Should I disable it in ruby/spec?

@eregon
Copy link
Member

eregon commented Jan 29, 2026

I think the BSD failure is just a run on an old commit, I expect it's fixed when the CI runs on a newer ruby/ruby commit.

@trinistr
Copy link
Contributor Author

Alright, we will see, I guess.

@eregon
Copy link
Member

eregon commented Jan 29, 2026

Mmh indeed it still happens and only on OpenBSD, I'll exclude that spec on OpenBSD for now.

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