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

fix zig fetch command in README #130

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from

Conversation

marler8997
Copy link

All URLs within build.zig.zon files must point to archives that never change. However, the zig fetch command in the README.md adds a URL that points to the devel git branch whose content changes whenever the devel branch is updated.

I've updated the README to instruct users to first pick a git SHA to reference if they want to add this project to their build.zig.zon file, then use that sha in their URL.

@AndrewKraevskii
Copy link

My issue about that was closed so I don't think it will be merged #120

@Not-Nik
Copy link
Owner

Not-Nik commented Aug 17, 2024

All URLs within build.zig.zon files must point to archives that never change

That sounds like a reasonable requirement, but I can't find it in official Zig documentation. If you can point me to it I'll gladly merge this.

@AndrewKraevskii
Copy link

AndrewKraevskii commented Aug 17, 2024

It turns out this works and adds latest commit hash in build.zig.zon. So no need to manually get hash from github

zig fetch --save git+https://github.com/Not-Nik/raylib-zig#devel
        .@"raylib-zig" = .{
            .url = "git+https://github.com/Not-Nik/raylib-zig?ref=devel#58df62807f62bef1db79538d04b37b9f79909d0a",
            .hash = "1220df9aa89d657f5dca24ab0ac3d187f7a992a4d27461fd9e76e934bf0670ca9a90",
        },

@Not-Nik
Copy link
Owner

Not-Nik commented Aug 17, 2024

That definitely seems like a better solution than what this PR proposes, however, I'm still not entirely convinced.

@AndrewKraevskii
Copy link

That definitely seems like a better solution than what this PR proposes, however, I'm still not entirely convinced.

I'm also not entirely convinced. Just saw it on kristoff_it's stream

marler8997 added a commit to marler8997/zig that referenced this pull request Aug 18, 2024
There's been some proliferation of dependency URLs that reference
mutable data such as links to git branches that can change.  This has
resulted in broken projects, i.e.

* https://github.com/RohanVashisht1234/raylib_rain_train/blob/9eef9de94c511f2eb4fe5db1d6abd574ee245c9b/build.zig.zon
* rcmagic/ZigFightingGame@4b64353

There's also disagreement about whether it's fine for URL's to point to
git branches, i.e.

Not-Nik/raylib-zig#130

This updates the docs to clarify that urls should reference immutable
data.
marler8997 added a commit to marler8997/zig that referenced this pull request Aug 18, 2024
There's been some proliferation of dependency URLs that reference
mutable data such as links to git branches that can change.  This has
resulted in broken projects, i.e.

* https://github.com/RohanVashisht1234/raylib_rain_train/blob/9eef9de94c511f2eb4fe5db1d6abd574ee245c9b/build.zig.zon
* rcmagic/ZigFightingGame@4b64353

There's also disagreement about whether it's fine for URL's to point to
git branches, i.e.

Not-Nik/raylib-zig#130

This updates the docs to clarify that urls should reference immutable
data.
@marler8997
Copy link
Author

I've created PR to update the ziglang docs here: ziglang/zig#21120

@kristoff-it
Copy link

kristoff-it commented Aug 18, 2024

zig fetch --save https://github.com/Not-Nik/raylib-zig/archive/devel.tar.gz has the problem that the Zig build system can't know that it is a moving target, so the URL gets copied verbatim into the build.zig.zon file and matched with the hash that results from the download at the time. This always works first time, but then if somebody else (or just CI) clones the project and has to download all dependencies from scratch, they will get an error when the tar archive results to a different hash.

zig fetch --save git+https://github.com/Not-Nik/raylib-zig#devel works because the devel ref gets first resolved to a commit and then written in the zon file after that. So once added it will always reference the same commit, which guarantees reproducibility. If after a while you want to update to latest devel, all you need to do is run the zig fetch command again, as that will update the contents of the zon file accordingly.

All URLs within build.zig.zon files must point to archives that never
change.  However, the zig fetch command in the README.md adds a URL
that points to the `devel` git branch whose content changes whenever
the `devel` branch is updated.

I've updated the README a url that zig will resolve to a SHA before it
writes it to the zon file.
@marler8997
Copy link
Author

marler8997 commented Aug 18, 2024

Thanks @kristoff-it that's a cool trick. I've updated the PR to use it. The user no longer needs to resolve the branch to a SHA themselves, now zig does it for them.

@AndrewKraevskii
Copy link

If accepted, github link style should also be changed in project_setup scripts

RaidoAun added a commit to RaidoAun/TowerDefense-v2 that referenced this pull request Sep 15, 2024
marler8997 added a commit to marler8997/zig that referenced this pull request Sep 30, 2024
There's been some proliferation of dependency URLs that reference
mutable data such as links to git branches that can change.  This has
resulted in broken projects, i.e.

* https://github.com/RohanVashisht1234/raylib_rain_train/blob/9eef9de94c511f2eb4fe5db1d6abd574ee245c9b/build.zig.zon
* rcmagic/ZigFightingGame@4b64353

There's also disagreement about whether it's fine for URL's to point to
git branches, i.e.

Not-Nik/raylib-zig#130

This updates the docs to clarify that urls should reference immutable
data.
marler8997 added a commit to marler8997/zig that referenced this pull request Sep 30, 2024
There's been some proliferation of dependency URLs that reference
mutable data such as links to git branches that can change.  This has
resulted in broken projects, i.e.

* https://github.com/RohanVashisht1234/raylib_rain_train/blob/9eef9de94c511f2eb4fe5db1d6abd574ee245c9b/build.zig.zon
* rcmagic/ZigFightingGame@4b64353

There's also disagreement about whether it's fine for URL's to point to
git branches, i.e.

Not-Nik/raylib-zig#130

This updates the docs to clarify that urls should reference immutable
data.
marler8997 added a commit to marler8997/zig that referenced this pull request Oct 2, 2024
There's been some proliferation of dependency URLs that reference
mutable data such as links to git branches that can change.  This has
resulted in broken projects, i.e.

* https://github.com/RohanVashisht1234/raylib_rain_train/blob/9eef9de94c511f2eb4fe5db1d6abd574ee245c9b/build.zig.zon
* rcmagic/ZigFightingGame@4b64353

There's also disagreement about whether it's fine for URL's to point to
git branches, i.e.

Not-Nik/raylib-zig#130

This updates the docs to clarify that urls should reference immutable
data.
@Not-Nik
Copy link
Owner

Not-Nik commented Oct 2, 2024

I'm not too proficient in PowerShell scripts and I don't have a Windows machine available right now. Let's hope this works.

marler8997 added a commit to marler8997/zig that referenced this pull request Oct 5, 2024
There's been some proliferation of dependency URLs that reference
mutable data such as links to git branches that can change.  This has
resulted in broken projects, i.e.

* https://github.com/RohanVashisht1234/raylib_rain_train/blob/9eef9de94c511f2eb4fe5db1d6abd574ee245c9b/build.zig.zon
* rcmagic/ZigFightingGame@4b64353

There's also disagreement about whether it's fine for URL's to point to
git branches, i.e.

Not-Nik/raylib-zig#130

This updates the docs to mention that zig won't be able to use URLs if
their content changes.
Repository owner deleted a comment from batyrmuhadov Oct 13, 2024
@opzap
Copy link

opzap commented Nov 23, 2024

Hello, is this PR going to be merged? Is there anything else that needs to be done?

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