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

Add Zig to ubuntu-rolling image, CI test #12953

Merged
merged 2 commits into from
Aug 6, 2024

Conversation

andy5995
Copy link
Contributor

#12293 (comment)

@eli-schwartz

This could be reviewed and merged here or I could do a PR against the branch and fork used in #12293 ?

/cc @Akaricchi

@andy5995 andy5995 requested a review from jpakkane as a code owner March 10, 2024 08:58
@andy5995 andy5995 changed the title os_comp.yml: Add Zig test Add Zig to image, CI test Mar 10, 2024
@andy5995 andy5995 changed the title Add Zig to image, CI test Add Zig to ubuntu-rolling image, CI test Mar 10, 2024
Copy link
Member

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

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

Status update as people have gotten re-interested in zig lately:

This PR fails the CI, which is a problem. You cannot add another workflow to os_comp.yml if it fails. Especially if we have no timeframe for when it might start working. ;)

@@ -58,6 +59,29 @@ source "$HOME/.cargo/env"
rustup target add x86_64-pc-windows-gnu
rustup target add arm-unknown-linux-gnueabihf

# Zig
# Use the GitHub API to get the latest release information
LATEST_RELEASE=$(wget -qO- "https://api.github.com/repos/ziglang/zig/releases/latest")
Copy link
Contributor

@RossComputerGuy RossComputerGuy Aug 6, 2024

Choose a reason for hiding this comment

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

This is the wrong way to find the latest release and is broken. There's a JSON file which can be curl'd from the download page (https://ziglang.org/download/) and each entry has artifacts.

Edit: Thought it was broken because I misread a line down, this works but it still probably is a better idea to query the index.json from the download page in case the download URL changes in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Not only isn't it broken, your reimplementation using the ziglang.org json file is broken.

I grow weary of nixos-brained people messing things up because they don't understand the basics of design.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not only isn't it broken, your reimplementation using the ziglang.org json file is broken.

CI failure is coming from the image builder failing which is happening upstream. I'm running a local test to ensure this works locally, from terminal testing it appears to work.

I grow weary of nixos-brained people messing things up because they don't understand the basics of design.

Um ouch

Copy link
Member

Choose a reason for hiding this comment

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

Every distro seems to be cursed with a specific type of user that collects around it.

For Gentoo, of course, it is the people who insist on recompiling all software with -funroll-loops -fomg-optimize -Ofast.

For Arch, it is "btw I use arch" ricers who who only installed Arch in order to screenshot neofetch and are entirely unfamiliar with running any other aspect of Arch.

And for NixOS it is people who confidently report things are broken, cannot look at or produce logs, and wait for someone else to come point out that it isn't broken and something else is coincidentally broken at the same time but far away -- then ignore that and throw stuff at the wall until something sticks.

And frankly, I think it's pretty lame to tell someone their PR is broken because you "misread" it and didn't even try it, when the alternative changes you propose are actually detrimental to the design planning. In particular, your alternative inherently defeats the purpose of a CI for "project A" (meson) by instead running CI for "project B" (zig). Don't insult other contributors because you're sad that they didn't download random bizarroland nightly editions of a compiler instead of a stable release -- they had their reasoning for choosing stable releases and it's the same reason I share.

I'm sorry but this is just incredibly disappointing, and it is ALWAYS nixos users doing this stuff.

Whatever. I updated the PR myself to exclude setting os_comp.yml, in accordance with my original request that zig be installed in the CI beforehand. It also credits the original author.

Since you cannot update the CI image containers at the same time you start depending on them. Our CI is not currently architected in such a way as to allow updating the ci/ciimage/ definitions and have the os_comp.yml jobs use that in the same push -- you have to deliver the updated images to Docker Hub before CI will start running against them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dude what? I was trying to offer constructive feedback. So what if I misread something, just point that out and leave it be. I didn't insult anyone. Also, don't group every single NixOS user together. I didn't bring up NixOS being anything related to this subject. Your comment feels really uncalled for and if this is how you see contributors, based on what distro they use or what projects they contribute to then I don't feel like it's worth contributing to this project. Meson has always been my favorite build tool as CMake is annoying to use and GNU Make/autotools is even more difficult. I'm just looking to make things better by contributing.

Copy link
Contributor

@Akaricchi Akaricchi Aug 6, 2024

Choose a reason for hiding this comment

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

@eli-schwartz That was… unnecessarily mean?

Copy link
Member

@eli-schwartz eli-schwartz Aug 6, 2024

Choose a reason for hiding this comment

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

Also, don't group every single NixOS user together. I didn't bring up NixOS being anything related to this subject. Your comment feels really uncalled for and if this is how you see contributors, based on what distro they use or what projects they contribute to

Not quite... I've used Arch for over a decade and then Gentoo for about a year, so if I actually just saw contributors based on what distro they use I'd have to be pretty judgmental of myself too.

Dude what? I was trying to offer constructive feedback.

Well, you didn't say what is wrong with it. Also I had already offered feedback saying that I had reviewed the PR and the parts in ci/ were good but the parts in .github/ "need to wait until the other PR is merged". So clearly, "entirely wrong" and "is broken" are, at best, statements that need to be clarified due to other people not seeing the issue you saw.

But ultimately, I have to say that it goes back to my comment above about "It also credits the original author."

Because if you take someone else's PR, tweak it slightly, and then submit it under your own name with you as the author, then... let's just say it predisposes me to view everything in the worst possible light. And while I could understand someone making a mistaken analysis in good faith and gently correct it, the story becomes a bit different when dealing with open source contribution laundering.

Please take this stuff seriously.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, you didn't say what is wrong with it.

That is true, I did make that mistake. I thought this would've 404'd but I misread it.

But ultimately, I have to say that it goes back to my comment above about "It also credits the original author."

The changes I made are a lot simpler and different but both implementations are only a few lines. That PR also references this one. I was originally going to cherry-pick it but it was just easier to start that commit from scratch. My implementation also works on aarch64 and x86_64 so that's a plus.

Please take this stuff seriously.

I am, it really feels berating getting this kind of feedback. I already owned up to the mistake I made by editing the message and clarifying on Matrix. Getting this kind of feedback without asking "why is it broken" or how my reimplementation is broken as well doesn't seem constructive. All I've seen is walls of text with little to no substantial feedback.

@eli-schwartz
Copy link
Member

@andy5995 ignore the nixos review please -- if you can update based on my review that will be fine...

andy5995 and others added 2 commits August 6, 2024 01:15
Signed-off-by: Eli Schwartz <eschwartz93@gmail.com>
[Eli: do not add to CI tests as this is only a preparatory PR]
They have recently upgraded to libgcrypt 1.11 and it has inherited the
gpg suite migration to pkg-config.
@eli-schwartz eli-schwartz merged commit 21eda4d into mesonbuild:master Aug 6, 2024
30 of 37 checks passed
@andy5995
Copy link
Contributor Author

andy5995 commented Aug 6, 2024

@RossComputerGuy I appreciate your effort and good intentions. Thank you.

@eli-schwartz Thanks for fixing it up.

@andy5995 andy5995 deleted the ci-add-zig-pr12293 branch August 6, 2024 07:24
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.

5 participants