Skip to content

Conversation

zeriyoshi
Copy link
Contributor

The current GitHub Actions job or script names are not normalized, and in particular the name x32 is very confusing with the Linux x32 ABI.

In this PR, we have changed to the more common names i386 and amd64. The amd64 is also called x86_64, but since i386 is not called x86, we see no reason not to call x86_64 amd64.

@zeriyoshi zeriyoshi requested a review from iluuu1994 February 23, 2023 13:31
@zeriyoshi
Copy link
Contributor Author

Linux x32 ABI: https://en.wikipedia.org/wiki/X32_ABI

@iluuu1994
Copy link
Member

iluuu1994 commented Feb 23, 2023

I can see that the naming is confusing but don't personally consider it a big deal. TBH I always thought AMD64 was a confusing name, so I would personally prefer something like x86_32 and x86_64 (or x86 and x64 if you prefer). If we do want this change, it needs to target PHP-8.0 and merged upstream. This is because the scheduled workflows on GitHub actions will use the branch-local actions instead of the ones from master.

@zeriyoshi
Copy link
Contributor Author

OK, I don't have a particular preference for naming conventions either. I just feel that x86_32 should be left as the more generic x86, but what do you think?

How about changing i386 to x86 and amd64 to x86_64? The x86_64 has become more versioned in recent years due to the addition of extensions over the years, and there is a distinction between v1, v2, and v3, etc., depending on the level of available extended instructions.

https://en.wikipedia.org/wiki/X86-64

@iluuu1994
Copy link
Member

x86 and x86_64 would be fine for me, although I guess I would prefer keeping x64 for 64-bit (reduces the amount of changes, and is also unambiguous).

@zeriyoshi
Copy link
Contributor Author

zeriyoshi commented Feb 23, 2023

This is a very complicated issue, but the notation x86 and x86_64 is probably the least confusing.

Similarly, I think it would be better to change the current MACOS to MACOS_X86_64, ASAN to ASAN_X86_64 and FREEBSD to FREEBSD_X86_64, can i do them at the same PR?

(I found this situation when I was thinking about ARM support for CI and wanted to get the architecture name right)

@zeriyoshi
Copy link
Contributor Author

Currently, there is no way to run CI for free using an ARM instance via QEMU, and it seemed unlikely that the original goal would be achieved. Azure seems to offer it, but that is probably no longer available. (I know nothing about the relationship between PHP Group and MS.)

As for macOS, it appears that it is already scheduled to be offered:
github/roadmap#528

@iluuu1994
Copy link
Member

@zeriyoshi We moved away from Azure Pipelines. I think MS is mainly focusing on GitHub actions. I suggest you try Cirrus, they also offer ARM.

@zeriyoshi
Copy link
Contributor Author

@iluuu1994
Thanks! I will investigate this! This PR will only change the naming in preparation for it.

@zeriyoshi
Copy link
Contributor Author

@iluuu1994
Have we left Travis CI as well? It was probably the only environment that could test the s390x architecture, but it seems to no longer be running.

@iluuu1994
Copy link
Member

@zeriyoshi Yeah, we only use Travis for architectures that there are no alternatives for.

but it seems to no longer be running.

Oh. I didn't notice. That's unfortunate. I'll have a look soon.

@Girgias
Copy link
Member

Girgias commented Feb 23, 2023

@zeriyoshi Yeah, we only use Travis for architectures that there are no alternatives for.

but it seems to no longer be running.

Oh. I didn't notice. That's unfortunate. I'll have a look soon.

I don't remember what was the reason again, but I think it was because Travis was being really flaky and not starting the builds properly

@iluuu1994
Copy link
Member

iluuu1994 commented Feb 23, 2023

Builds have been temporarily disabled for public repositories due to a negative credit balance. Please go to the Plan page to replenish your credit balance or alter your Consume paid credits for OSS setting.

I created an issue. #10683

@zeriyoshi zeriyoshi changed the base branch from master to PHP-8.0 February 23, 2023 14:57
@Girgias
Copy link
Member

Girgias commented Feb 23, 2023

@zeriyoshi You need to rebase the branch locally onto PHP-8.0 and force push

@zeriyoshi
Copy link
Contributor Author

@Girgias
I was confused. I will redo everything :)

@iluuu1994
Copy link
Member

iluuu1994 commented Feb 23, 2023

Note there are more job names in .github/nightly_matrix.php, can you adjust those too? Nevermind actually, those are just suffixes. LGTM then. Feel free to squash and merge.

@zeriyoshi
Copy link
Contributor Author

@iluuu1994
The test fails on macOS, but apparently the PHP-8.0 branch already fails the test. Is it safe to merge it as is?

Also, it has the Waiting on RM tag, does it need to be reviewed by the release manager for each version?

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Weird, not sure why these macOS tests fail but I doubt its related.

@@ -233,7 +233,7 @@ jobs:
-d opcache.jit=1205
- name: Verify generated files are up to date
uses: ./.github/actions/verify-generated-files
COVERAGE_DEBUG_NTS:
COVERAGE_X86_64_DEBUG_NTS:
Copy link
Member

Choose a reason for hiding this comment

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

I guess more consistent would be LINUX_X86_64_DEBUG_NTS_COVERAGE. Or we could drop everything and just do COVERAGE, because the arch and configs are not very relevant for these builds. Same with the jobs below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, as far as coverage is concerned, it has little to do with architecture, so I'll take it back.

@zeriyoshi
Copy link
Contributor Author

I think the test fails on macOS probably due to the version of ICU. This is not related to this fix, so we will attempt to fix this separately.

@zeriyoshi
Copy link
Contributor Author

@iluuu1994
Fixed and squashed. Please review.

Can I do the merge work? Probably this is a conflict in PHP-8.1, PHP-8.2, which needs to be resolved properly.

@mvorisek
Copy link
Contributor

mvorisek commented Feb 26, 2023

I prefer x64 instead of x86_x64 to keep it the same length as x86 and easier to read/locate.

@zeriyoshi
Copy link
Contributor Author

@mvorisek
OK, Windows seems to use that naming convention, so it might be a good idea to adapt it. I'll fix it that way.

@zeriyoshi zeriyoshi closed this May 25, 2023
@zeriyoshi
Copy link
Contributor Author

This may not be very important. We do not have time to respond to it, so we will close it for now. Thank you!

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.

4 participants