« first day (4475 days earlier)      last day (486 days later) » 

9:52 AM
@cmb Do you have an info on the mismerged branches? I'm about to check now.
 
cmb
@Derick Mismerged branches? Can you detail, please?
 
You told me last week... that 8.0 wasn't merged into 8,1 or so?
But that's because he messed it up, by merging 8.1 INTO 8.0, instead of the other way around
 
Is this about Dmitry's revert?
 
that too, yes
sorry, which revert?
 
9:55 AM
also, I was wrong, it doesn't seem you messed anything up - sorry
 
I might've. The merge from 8.0 into 8.1 went cleanly, but unintentionally reverted the OpenSSL 3 support by that, because that was reverted for 8.0, but likely shouldn't be reverted in 8.1.
 
the branches look all fine to me.
so I am not sure what Dmitry is about (in a private emal)
 
So not technically a merging failure, but an inclusion of a change that should've been dropped.
 
cmb
@Derick There was a merge glitch (by GPB), but that had been resolved an hour later.
 
ah
 
cmb
9:57 AM
@Derick This may be about github.com/php/php-src/pull/10220 (and some related PRs). I just tried to revert, but got a conflict. I can have a look later, if nobody beats me to it, but it seems Dmitry is already very annoyed.
 
Yes, he wrote an email to the PHPF people and Roman :-)
To be fair... I am not fan of these commits either.
 
cmb
Me neither. I think that should be RFCd.
 
But Dmitry did agree to them ;-)
https://github.com/php/php-src/pull/10216#issuecomment-1375140255
I'm tempted to revert all of them, and make him go through an RFC.
But there are so many :-/
 
10:46 AM
no idea how to report or what is actually happening here: since 8.0, when registering an error handler that turns the error into an ErrorException() and throws it (so the handler does not return false) the exception is normally thrown unless it is triggered by a warning in proc_open() (the error handler is still invoked and I can write to standard streams) but the exception is gone.
the warning in proc_open() is interesting at the point that the error is reported for the file/line proc_open() is called, however the PHP script already continued. Adding some information to state, be it global, superglobal or object (haven't tried statics) it is as-if the state is restored after the error-handler call and in the error-handler callback as-if it is at the time of proc_open().
the warning in proc-open() is causing exit status 127 as the command is not found, to add some more context about it (e.g. the command is not in PATH).
in 7.4 the error handler is not called at all. before 7.4 the exception is thrown.
 
cmb
@Derick I'm afraid that Dmitry only looked at that PR and didn't think/read about more changes.
And yes, there are too many commits. IMHO, PRs should be squashed, unless there is a very good reason not to. There still can be multiple PRs.
 
yeah
well, I actually disagree with that - a commit should be a specific change that belongs together
but that's not the case here...
 
cmb
11:04 AM
@TimWolla Yeah, github.com/php/php-src/commit/… should be reverted; there shouldn't be OpenSSL 3 support in PHP 8.0. I think that was discussed somewhere on GH.
 
@cmb That revert you linked is in 8.1.
If OpenSSL 3 should be supported in 8.1 the revert is correct (but would've deserved a better commit message).
 
cmb
@TimWolla Ah, right! A better commit message would have been helpful. Anyway, seems to be good now.
 
cmb
11:47 AM
@Derick I agree; but I also think that a PR should be about a specifc change that belongs together. Anyway, I have no hard feelings about this, but a commit which had a PR should indicate that PR, and having a single commit makes that easier. To revert these include changes, I had to check the individual commits via there title manually.
 
Oh, you reverted them?
(also, it's "their" :-) )
 
cmb
Yeah, English is a hard language. It can be understood through tough, thorough thought, though.
16
 
How long did you work on that? :-D
 
cmb
Saw it on the bird site yesterday, and googled it.
 
12:12 PM
@cmb Max specifically asked for commits not to be squashed in the past, but I completely agree with you.
 
@cmb He does too many commits in one PR, but these changes are all independent from one an other so that's why I didn't squash them
 
cmb
Okay, there may be good reasons to keep the commits, but I'm always concerned regarding git bisect (can we be reasonable sure that every single commit works?) And of course, if changes to the PR are requested and done, one would need to patch old commits, that get's messy, and prevents the possibility to only review these changes (due to force-push). But that is my opinion; YMMV. :)
 
Honestly, if the issue was just about some OpCache change that broke or DTrace the specific header commit should have been reverted not the whole list
 
Saying master didn't build was a little overdramatic. I have never even heard of dtrace and didn't know we cared about it. If we do, CI should cover it.
 
Also, I don't know why on Earth Dmitry is asking for an RFC for these changes. Because by that logic I may as well push through an RFC to get const qualifiers added to a bunch of OpCache functionssomething Dmitry finds annoying because it makes his merging up with his IR/JIT 2.0 version harder
 
12:20 PM
@cmb only if every commit is fully functional which is hard to verify in a pr. If they aren't it would make bisect more difficult.
 
cmb
I think the dtrace break was only when Dmitry noticed the amount of changes, and IMHO going through an RFC shouldn't hurt (yeah, more work, but at least there would be a community decision).
 
Oh sorry, that's what you said :) misread something
 
cmb
And like I said earlier, these include changes could break a lot of extensions, and there is a lack of bandwidth regarding their maintenance, anyway.
 
Sure, but Max got told off about these const changes, so by the same logic if there is a community agreement about adding them then it should go. Something which feels completely unproductive as an RFC conversation
@cmb Better now than in May, but yes.
 
@cmb but also, I wonder if most people with voting rights are actually informed about the code base to make these decisions, when the people that are working on it supposedly aren't
 
cmb
12:25 PM
@IluTov I hope that those who don't really work on php-src or extension, will refrain from voting. :)
 
People voting without all of the knowledge needed to make an informed decision... sir, surely you jest anyone would do such a thing... /s
 
cmb
And IMHO, RFC shouldn't ideally be about the vote, but rather about the discussion (and documentation). :)
 
@cmb Sure, I don't object :) I'm just not sure internals is the best medium, Dmitry himself said he doesn't read it.
 
@IluTov Bruh
 
12:50 PM
There's a JIT 2.0?
 
@IluTov Yeah. What is dtrace? :-D
@MarkR Yes.
 
My eyes already glazed over.
 
The comments on each include why they are there is a silly idea too. /cc @Girgias
 
cmb
12:55 PM
DTrace is a comprehensive dynamic tracing framework originally created by Sun Microsystems for troubleshooting kernel and application problems on production systems in real time. Originally developed for Solaris, it has since been released under the free Common Development and Distribution License (CDDL) in OpenSolaris and its descendant illumos, and has been ported to several other Unix-like systems. DTrace can be used to get a global overview of a running system, such as the amount of memory, CPU time, filesystem and network resources used by the active processes. It can also provide much more...
 
I'll disagree on that, but anyway should we have a follow up RFC on that one when it gets accepted to remove the comments?
 
@cmb FWIW: You can skip commits in git bisect. If all the related commits come right after each other (as it is when rebase+merging on GitHub) then this shouldn't even make the result unclear.
 
Also I am getting extremely tempted to be that petty person and make an idiotic RFC about adding those const qualifiers into OpCache
 
cmb
@Girgias Ideally, we find a solution that suits everyone (or at least a supermajority).
 
Personally I'm generally not a fan of squashed PRs, but that requires commit discipline by the PR author.
 
12:57 PM
GIT is hard enough already :-)
 
@cmb Sure, something I thought we had prior to merging all of this
 
cmb
@TimWolla Yeah, I know that. But I'm occassionally bisecting between minor version (> 1000 commits), and it's not easy to keep the overview.
 
Do you know about the 'git bisect view' command? I find this very helpful to keep the overview over a bisect.
 
cmb
@Girgias I think Dmitry only (partially) agreed to the first commit; apparently, he was not aware that there was more to come.
 
> DTrace is a magician that conjures up rainbows, ponies and unicorns — and does it all entirely safely and in production!
 
12:58 PM
@TimWolla but building, testing, just to see it's borked is also annoying
 
cmb
^ that, too
 
I am the one who reviewed those PRs, if you want to blame someone, blame me. I looked at it and every commit seemed self contained
 
@Derick at least that's what dtrace.org/blogs/about claims. I dabbled with it some time ago for obvious reasons but felt it inadequate for distributed tracing.
 
cmb
Nobody needs to be blamed. It's obviously okay to do it one way or the other; I don't think we have any rules about how to merge exactly.
 
It's easy to blame somebody. There seems to be some consensus that this should go an RFC. It's a learning process.
 
1:01 PM
@IluTov Yeah, absolutely. As said, it requires commit discipline to make not-squashing useful. A "fix dumb bug" commit helps nobody, because that's obviously broken.
 
Also, I definitely disagree that every commit needs to be reviewed by 10 people. Dmitry says he doesn't have time so how would that even work. Master is tested for a year so for a feature to he slightly broken for a day doesn't seem like a huge problem.
 
cmb
^ that. But I don't like force-pushes on PRs; if I had already reviewed, I'm not able to quickly see the changes (if at all), so might need to review again.
 
Is there a reason not to use github's squash and merge so only the one commit gets added to master?
 
@cmb I usually commit fixes for this reason, keep them for a while and only do fixup later so the reviewer had a chance to see them.
 
@IluTov Dmitry is hard to work with ngl, there is stuff that is obviously broken for extensions (e.g. github.com/php/php-src/pull/9218) but he doesn't want to fix it because of performance. But this somehow breaking extensions is now an issue which is easily remedied on the extension author side
@MarkR I merge those PRs with the Rebase and merge feature of GH, I also use the squash and merge for PRs that go into master
 
1:05 PM
Also, worst case, Github allows looking at a diff between the previous commit and the force pushed one.
 
cmb
@IluTov No, of course there is no need for approval of many contributors. But if someone objects even in hindsight, more discussion is required. :)
Oh, didn't know that (diff between previous and force-pushed)
 
@cmb Sure, I'm not trying to kill discussion. But this discussion didn't feel particularly fruitful.
Dmitry can be quite negative, and I'm sure it discourages quite a few people from contributing.
 
@MarkR I never ever use the UI for merging anything. Because it randomly does things I hadn't intended.
 
@cmb it's pretty hidden, you need to click on the "cmb [force-pushed] a commit"
 
cmb
ta
 
1:11 PM
But it also shows commits you rebased onto, so it's not perfect
 
cmb
@IluTov I think he's always short on time, isn't interested in most minor stuff, but has very strong opinions regarding ZE and its performance. And then there is a bit of language barrier. :)
@Girgias I came up with the potential extension BC break; that wasn't Dmitry. :) sorry for double ping
 
@cmb Fair, I mean the language barrier does make it hard
 
hah lol
nearly 20 years ago...
 
1:52 PM
@Gordon Long time no see \o/
 
@StatikStasis true. I am in meetings all the time nowadays.
 
o/
 
@Gordon Me too... meetings about meetings. Seriously...
\o
 
@Ekin Did @Ekin get married yet?
5
 
I did :-)
12
 
1:54 PM
\o/
 
Congratulations! \o/
 
@Ekin Congrats Mrs. PeeHaa!!! //cc @PeeHaa
 
Oh waitm
 
thanks!
 
you married @PeeHaa, right... so congrats to @PeeHaa and ummm… well… much strength or something to you ;) j/k congrats
 
1:56 PM
6th day of being his wife, phew, already doubting my life choices :D
 
lol
 
=D
I think on the 7th day God rested from his labor... so you deserve a break too at this point.
 
cheers to the slight possibiliTy of that :-) (freaking atext)
 
@Ekin I am pretty sure we all warned you though ;)
 
yup, you all sure did warn me :)
can't believe that was 5-6 years ago
 
cmb
2:17 PM
@Derick Xdebug uses CirclCI; are you aware of circleci.com/blog/january-4-2023-security-alert?
 
2:54 PM
I saw a headline... let me check. cheers
I don't think I have any secrets on it though
 
3:30 PM
@cmb What would you say to that? github.com/php/php-src/pull/10339/…
 
cmb
@IluTov There are no workers on Windows.
 
@cmb Then I'm confused about this comment. github.com/php/php-src/pull/10276#issuecomment-1383868696
(I have no knowledge about PHP on Windows)
 
cmb
I'm having a closer look. Thanks for pointing this out!
 
@cmb Np! I didn't think you'd like such a warning :D
 
cmb
I mean, OPcache is broken on Windows; it's generally only held together with a lot of duct tape. :)
 
3:40 PM
@cmb That sounds reassuring :)
We did run many PHP websites on Windows, where a common "fix" was recycling the application pool for no apparent reason ^^
Interestingly, our company CMS only worked on Windows for a long time. For very dumb reasons, that is, they used DIRECTORY_SEPARATOR for building class namespaces :D
 
cmb
@IluTov The background is (besides missing fork(2)) continously better adoption of ALSR on Windows (Win7 introduced that behind a flag; later it was mandatory, and randomized more). However, FCGI runs completely separate processes, so we would need to make sure that these have the same memory layout; currently, we're only checking whether execute_ex is at the same address.
That's the reason why file_cache_fallback is recommended on Windows (although that doesn't really improve performance).
 
Pfff, and our boss always blamed us.
 
cmb
See also github.com/php/php-src/blob/… (I have still no idea how it could be possible to force each process to have the same address, and how that would work for DLLs).
It's probably best not to use PHP for the Web on Windows in production environments.
Also, file system is super slow, and WinCache development has ceased long ago.
 
4:00 PM
@cmb Sounds lovely
 
4:46 PM
@cmb I think Georges RFC is only about accessing the fd. What you do with that - PHP does not really expose facilities for that except php://fd/ - but you just don't use that if you don't know what you're doing :-D
 
@bwoebi I forgot stream_get_meta_data() existed
 
cmb
Well, I don't think anybody wants file_descriptor() just to be able to print out some numbers; it's more likely that they want to do something with it. And if they read from the fd, the stream resource would be broken. Okay, can be documented, and since everybody reads the documentation, there is no problem. ;)
Note that I'm not strictly against that feature. :)
 
> This is useful when interacting with a USB device.
An example in the RFC itself would be useful.
 
@cmb tbf, talking to Pierre what they were doing was replacing a Python lib they were using at Karafun just to interact with a USB device that handles the microphones of their Karaoke machies (as fair as I remember) to then send shell commands
 
cmb
Okay, but can't they use PECL/DIO instead? Maybe it's just that they don't know about that extension.
 
4:55 PM
fwiw - I would also like that feature, to play around with FFI talking to io_uring.
without having to hack my way into the zend engine with FFI too.
so, maybe the extension would do it, but ... it's such a small thing to expose and I would just need the file descriptor.
@Girgias and has the side-effect that you don't have to throw :-D you'd isset()-check for it
 
I mean, I still think throwing on gibberish is better than that -1 return :)
But I can add it to that, have the function and add a method to SplFileInfo
 
5:14 PM
@Girgias gibberish, yes. But you might have a php://temp which is also a stream, just not a fd backed one
 
 
1 hour later…
6:33 PM
@brzuchal Potentially. Still undecided.
 
@hakre I now could minimalize the example of throwing a non-catchable, non-fatal exception in php 8 having earlier today. 3v4l.org/gXjcI
 
No worries. The idea was already seeded. Let it grow now :P
 
would this be worth to report? it looks like there is some internal error handling and it is perhaps somewhat good this can not throw.
as the 'proc_open(): Exec failed: No such file or directory' message comes much later.
 
cmb
6:51 PM
@hakre If in doubt, file a report. :)
 
cmb
ta
 
 
1 hour later…
8:48 PM
@bwoebi @LeviMorrison we finally gotten raound detailing our crash, though no success on getting it reproduced outside a large scale symfony app github.com/php/php-src/issues/10346
 
@brzuchal How would it help with ADTs?
 
9:49 PM
I'm going through analyser reports. This code (github.com/php/php-src/blame/PHP-8.1/ext/pdo/…) looks broken because the result of "pdo_hash_methods" doesn't matter. But then again it has been here for 18 years and no one seems to have noticed bad behaviour here, so there might not be an actually triggerable bug here and I'm not knowledgable enough in this area...
Please ignore what I said, it's correct
sorry about the noise
(the pdo_hash_mehtods does initialise it correctly)
 
 
1 hour later…
11:01 PM
@SaifEddinGmati I wonder if combining $() with the previous PFA syntax would resolve the issues we ran into there.
Something like:

$() indicates a closer is made over an expression. If at compile time the engine can determine that it's just a single function call, do the same optimizations that Joe had done before. If not, make a full-on closure and if that's a bit slower, so be it.
But then use the ... and ? syntax worked out before.
Since IIRC, the main objection was the complexity of the "this is a PFA" detection, which happened inside every function call.
 

« first day (4475 days earlier)      last day (486 days later) »