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.

Sign up for GitHub

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

Jump to bottom

Refactor glob.c to be Windows specific #15564

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from
Draft

Refactor glob.c to be Windows specific #15564

wants to merge 11 commits into from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Aug 23, 2024

As far as I can tell, we never use glob.c to implement support for *NIX based OS that do not have glob() as such, let's make it truly Windows specific and remove some weird cross-section dependences on Zend and php.h.

@Girgias Girgias requested a review from cmb69 August 23, 2024 22:21
@Girgias Girgias force-pushed the windows-glob-circular-dependencies branch from 463c04c to 5709e0b Compare August 23, 2024 22:51
@cmb69
Copy link
Member

cmb69 commented Aug 23, 2024

Are you planning to do this for PHP 8.4? If so, I'll have a closer look as soon as possible (might still take a couple of days due to long backlog). Otherwise I'm not going to bother with this PR during the next weeks (there are still so many issue to solve for PHP 8.4).

@Girgias
Copy link
Member Author

Girgias commented Aug 23, 2024

This is low priority and not intended for 8.4.

I'm just in my janitor phase of going through different parts of the codebase. :)

@Girgias Girgias force-pushed the windows-glob-circular-dependencies branch from 9d9aa7d to f96ce00 Compare August 23, 2024 23:55
@Girgias Girgias force-pushed the windows-glob-circular-dependencies branch from 83cfb11 to 4c3ad1f Compare August 24, 2024 01:03
@Girgias Girgias force-pushed the windows-glob-circular-dependencies branch from 4c3ad1f to 2ee1742 Compare August 24, 2024 01:27
@nielsdos
Copy link
Member

I don't recommend doing this. This isn't our code and it is quite behind upstream. We should pull in upstream changes and probably not refactor this to keep pulling upstream easy.

@cmb69
Copy link
Member

cmb69 commented Aug 24, 2024

This isn't our code and it is quite behind upstream. We should pull in upstream changes and probably not refactor this to keep pulling upstream easy.

Good point, but would be upstream? I don't think we ever pulled from whatever upstream repo there is, and the introducing commit doesn't mention where this had be taken from, either. And some quick Web search gives several results.

@nielsdos
Copy link
Member

This is glob from OpenBSD, and it was last synced around 2002.

@cmb69
Copy link
Member

cmb69 commented Aug 24, 2024

This is glob from OpenBSD, and it was last synced around 2002.

So upstream would be https://github.com/openbsd/src/blob/master/lib/libc/gen/glob.c and https://github.com/openbsd/src/blob/master/include/glob.h. Syncing might be some work then; still a reasonable thing to do (and maybe to provide patches against upstream), but we also need to document that in https://github.com/php/php-src/blob/master/CONTRIBUTING.md#php-source-code-directory-structure.

@nielsdos
Copy link
Member

I think that we need to sync even on stable versions to pull in some fixes.

@NattyNarwhal
Copy link
Member

FWIW, I'm actually looking into issues with the system glob on AIX reported by users, and considering replacing it with i.e. NetBSD glob. It does miss some functionality like the brace matches, so it might be useful too wire it up to Unix.

However, the way I was looking at doing it was putting said better glob in a library, provide its own name for functions, and having my own glob.h in the include path ahead of the system one. If PHP offers its own glob replacement, that would be easier though.

@jimwins
Copy link
Member

jimwins commented Aug 24, 2024

Having tried to clean up the glob() documentation a bit recently (PR php/doc-en#3647), it does seem entirely likely that PHP would be better of with it's own implementation (based on some BSD code?) used everywhere instead of ever using the system one, because those old Unix variants just make it annoying. If we have to have an implementation around for Windows anyways, may as well make it complete and use it on Unix, too.

@cmb69
Copy link
Member

cmb69 commented Aug 24, 2024

Yeah, that's an old (adapted) version of OpenBSD's glob (should be updated, though). Using it instead of the system library's glob() would have the additional advantage that we might be able to adapt it to the streams layer (what has been requested in the past), besides having identical functionality on all systems (and fnmatch() could eventually also use the implementation).

cmb69
cmb69 requested changes Aug 25, 2024
View reviewed changes
Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Might be best to decide first where to go with our internal glob implementation, before doing any changes at all.

@Girgias
Copy link
Member Author

Girgias commented Aug 25, 2024

I am fine with having this become our standard glob() implementation, but in this case this should be moved out of the win32 folder.

@cmb69
Copy link
Member

cmb69 commented Aug 25, 2024

Yeah, sure, but lets formally discuss this first (might need an RFC).

@NattyNarwhal
Copy link
Member

I'd be willing to help with an RFC for the next release if needed.

@Girgias
Copy link
Member Author

Girgias commented Aug 27, 2024

I'd be willing to help with an RFC for the next release if needed.

This would be appreciated! I brought it up in the last Foundation dev meeting, and we seem to be in agreement that this would be a good way forward.

devnexen
devnexen reviewed Aug 30, 2024
View reviewed changes
win32/glob.c
}

/* Free allocated data belonging to a glob_t structure. */
PHPAPI void globfree(glob_t *pglob)
PHP_WIN_GLOB void globfree(glob_t *pglob)
{
register int i;
Copy link
Member

Choose a reason for hiding this comment

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

nit: I question register usefulness wdyt ?

Copy link
Member

Choose a reason for hiding this comment

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

Me too, but we should stick as close to upstream as possible. and need to update to latest (where this appears to be gone; also it's no longer an int).

And see https://github.com/php/php-src/pull/15640/files#diff-8ad7174b356b2ed6831802d66a1e2ed4372cc20837f2346fae86d49297d40d36R973 ;)

NattyNarwhal added a commit to NattyNarwhal/php-src that referenced this pull request Nov 26, 2024
@NattyNarwhal
Update OpenBSD glob implementation for Windows
350e136 
We're considering making this used as a glob implementation on POSIX as
well, but first, we should rebase it from the latest version of OpenBSD.

See conversation in phpGH-15564.
NattyNarwhal added a commit to NattyNarwhal/php-src that referenced this pull request Nov 26, 2024
@NattyNarwhal
Update OpenBSD glob implementation for Windows
d9cd99b 
We're considering making this used as a glob implementation on POSIX as
well, but first, we should rebase it from the latest version of OpenBSD.

This also adds a new internal header (charclass.h) for glob.

See conversation in phpGH-15564.
@NattyNarwhal NattyNarwhal mentioned this pull request Nov 26, 2024
Merged
NattyNarwhal added a commit to NattyNarwhal/php-src that referenced this pull request Nov 27, 2024
@NattyNarwhal
Update OpenBSD glob implementation for Windows
affa4a7 
We're considering making this used as a glob implementation on POSIX as
well, but first, we should rebase it from the latest version of OpenBSD.

This also adds a new internal header (charclass.h) for glob.

See conversation in phpGH-15564.
NattyNarwhal added a commit to NattyNarwhal/php-src that referenced this pull request Nov 27, 2024
@NattyNarwhal
Update OpenBSD glob implementation for Windows
48fbd32 
We're considering making this used as a glob implementation on POSIX as
well, but first, we should rebase it from the latest version of OpenBSD.

This also adds a new internal header (charclass.h) for glob.

See conversation in phpGH-15564.
NattyNarwhal added a commit to NattyNarwhal/php-src that referenced this pull request Dec 3, 2024
@NattyNarwhal @cmb69
Update OpenBSD glob implementation for Windows
3aafb04 
We're considering making this used as a glob implementation on POSIX as
well, but first, we should rebase it from the latest version of OpenBSD.

This also adds a new internal header (charclass.h) for glob.

See conversation in phpGH-15564.

Co-authored-by: Christoph M. Becker <[email protected]>
NattyNarwhal added a commit that referenced this pull request Jan 10, 2025
@NattyNarwhal @cmb69
Update OpenBSD glob implementation for Windows (#16948)
97a26ae 
We're considering making this used as a glob implementation on POSIX as
well, but first, we should rebase it from the latest version of OpenBSD.

This also adds a new internal header (charclass.h) for glob.

See conversation in GH-15564.

Co-authored-by: Christoph M. Becker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devnexen devnexen devnexen left review comments

@cmb69 cmb69 cmb69 requested changes

@TimWolla TimWolla Awaiting requested review from TimWolla TimWolla will be requested when the pull request is marked ready for review TimWolla is a code owner

@zeriyoshi zeriyoshi Awaiting requested review from zeriyoshi zeriyoshi will be requested when the pull request is marked ready for review zeriyoshi is a code owner

Assignees
No one assigned
Labels
Extension: random OS: Windows Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Girgias @cmb69 @nielsdos @NattyNarwhal @jimwins @devnexen