Skip to content

Add jfinkhaeuser/json-rpc #249

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

Closed
wants to merge 8 commits into from
Closed

Add jfinkhaeuser/json-rpc #249

wants to merge 8 commits into from

Conversation

jfinkhaeuser
Copy link

@eli-schwartz
Copy link
Member

Since there is an upstream meson.build you don't need to have a patch_directory key as that is only for adding third-party overlays that add a meson.build on top of the upstream tarball.

Remember to update releases.json before taking this out of draft mode. :)

@jfinkhaeuser
Copy link
Author

Hmm, so the requirement that the source URL must contain the version bites me here. Whatever attachment I manually upload won't have that. And whatever automatically generated source tarball gitea generates won't have the project version in the subdirectory within the tarball. That's a little awkward.

@eli-schwartz
Copy link
Member

I haven't taken a good look at the project, what does the generated file do?

@jfinkhaeuser jfinkhaeuser marked this pull request as ready for review December 22, 2021 16:16
@eli-schwartz
Copy link
Member

eli-schwartz commented Dec 22, 2021

Oh wait a minute. You mean codeberg's autogenerated tarballs don't include the tag or hash that they correspond to in the root directory name? And the manually uploaded releases only provide download urls that have anonymous hashes?

What the heck, codeberg.

@jfinkhaeuser
Copy link
Author

I think this speaks louder than the description:

$ tar vft subprojects/packagecache/v0.1.1.tar.gz 
drwxrwxr-x root/root         0 2021-12-22 16:49 json-rpc/
-rw-rw-r-- root/root       321 2021-12-22 16:49 json-rpc/.gitignore
-rw-rw-r-- root/root       673 2021-12-22 16:49 json-rpc/.oclint
-rw-rw-r-- root/root     35147 2021-12-22 16:49 json-rpc/COPYING
lrwxrwxrwx root/root         0 2021-12-22 16:49 json-rpc/LICENSE -> COPYING
-rw-rw-r-- root/root      9151 2021-12-22 16:49 json-rpc/README.md
...
  1. Generated file name does not contain the project name.
  2. Generated file does not contain the version in the directory name.

But if e.g. I manually upload a file from meson dist, then that will fail the sanity checks because gitea gives it a weird attachment name.

@jfinkhaeuser
Copy link
Author

I may be able to work around this by tagging releases with the full package name. I haven't tried... let me check.

@jfinkhaeuser
Copy link
Author

Yes. So the tarball receives exactly the tag name, which means I can give it a more meaningful name. However, it won't put a version path inside the tarball contents. I can improve on this, but not to my liking.

You can see the download paths on the two releases at https://codeberg.org/jfinkhaeuser/json-rpc/releases - basically, I can't fix it all.

@jfinkhaeuser
Copy link
Author

Visual studio sanity checks also fail; that's a separate issue, though. I can disable windows, and/or wait until a later release with it.

@jfinkhaeuser
Copy link
Author

It's a gitea issue rather than a codeberg issue, for what it's worth: go-gitea/gitea#18078

It would probably also be interesting if meson/wrapdb could relax its requirements here. But I still think gitea should fix this, because it also doesn't help when managing release tarballs outside of meson or similar projects.

@eli-schwartz
Copy link
Member

The meson sanity check there is supposed to catch cases where people forget to bump the download url when bumping the version, so it generally makes sense to keep it in.

As far as gitea archive/url generation goes, the problem is not exactly what you think. The remote filename doesn't necessarily have to have both the repo name and the tag name, e.g. it doesn't on github or sourcehut either. What does happen is that the remote url contains content-disposition: attachment; filename=json-rpc-v0.1.1.tar.gz so a web browser or e.g. wget will rename the file when saving it.

meson supports this too, in a sense, because source_filename is allowed to rename the final destination name of source_url and many existing wraps do exactly that. (meson ignores content-disposition entirely, because it requires you to specify an output filename). This is important because many projects might have version v0.1.1.tar.gz and clobber each other.

The real problem is the lack of the tag or commit ref in the directory root of the unpacked tarball. This causes problems across the ecosystem, for example extracting a newer version into the same directory an older version was previously extracted to will clobber existing contents and the overlapping contents can cause misbehavior.

...

And of course release assets should have a stable url that doesn't require embedding a difficult-to-vet hash into the .wrap file. Some software forges use stable urls in the first place. Others use stable urls that you can embed in .wrap files or distro package recipes, which redirect to a hashed url used internally by the software forge.

@eli-schwartz
Copy link
Member

Windows is failing because you, like 99% of people, are gullible and think __cplusplus is supposed to say what std the code is compiling with.

https://docs.microsoft.com/en-us/cpp/build/reference/zc-cplusplus?view=msvc-170

Of course, the only logical and rational understanding of this macro is actually that it will always declare 199711L in order to avoid breaking code which Microsoft pinky swears does exist somewhere and definitely miscompiles unless the compiler reports that std and only that std.

@eli-schwartz
Copy link
Member

Really this should just be a default flag that meson passes. mesonbuild/meson#9125

"cpp_std=c++17"
],
"fatal_warnings": false
},
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of disabling warnings here?

Copy link
Author

Choose a reason for hiding this comment

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

I use specific warning flags rather than relying on meson doing what I want it to do.

Copy link
Member

Choose a reason for hiding this comment

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

Do you have a specific concern about what meson may do here?

source_hash = 88002cc87ae87d56c76e9e4bc46456198cb4dd048913d4395ee79c974e14df8d

[provide]
json_rpc = json_rpc_dep
Copy link
Member

@eli-schwartz eli-schwartz Dec 23, 2021

Choose a reason for hiding this comment

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

It seems that this provides a subproject accessible declared dependency, but does not install a pkg-config file for system use or for use in projects that use a build system other than meson.

Copy link
Author

Choose a reason for hiding this comment

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

Correct. I don't particularly mind pkg-config, but it's not very high on my priority list.

Copy link
Member

Choose a reason for hiding this comment

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

It's trivial to add, meson has a built-in exporter for it. :)

pkgconfig = import('pkgconfig')

pkgconfig.generate(foo_lib)

There are optional kwargs that can fine tune the metadata, for example giving the project description.

Copy link
Member

Choose a reason for hiding this comment

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

Just to note, adding a .pc file is not required. In fact if upstream does not provide one, then it makes sense to not add one at all, because otherwise people will start to depend on it existing which will cause problems.

@jfinkhaeuser
Copy link
Author

Windows is failing because you, like 99% of people, are gullible and think __cplusplus is supposed to say what std the code is compiling with.

Absolutely - I only work with MSVC when finishing up a project revision, it's a PITA otherwise. Turns out my VM doesn't have networking at the moment, and I don't care for this release to make the Windows build work. I can do that in a subsequent release. I'll find some way to deal with this better.

@jfinkhaeuser
Copy link
Author

As far as gitea archive/url generation goes, the problem is not exactly what you think. The remote filename doesn't necessarily have to have both the repo name and the tag name, e.g. it doesn't on github or sourcehut either. What does happen is that the remote url contains content-disposition: attachment; filename=json-rpc-v0.1.1.tar.gz so a web browser or e.g. wget will rename the file when saving it.

Well, if we're nitpicking, URLs don't have a filename component. So anything with file names comes via the content-disposition header or is guesswork by the client.

It doesn't matter, though, because the sanity check still fails because the URL does not contain a <package>-<version> pattern.

@eli-schwartz
Copy link
Member

eli-schwartz commented Dec 23, 2021

This is not nitpicking, but directly practical considerations.

Anyway, urls do have a filename component. Anything after the final /. It's not guesswork.
See for example any static site, ftp server, Apache homedir in ~/public_html/, etc.

Content-disposition overrides that, but not all download agents respect content-disposition e.g. curl / wget have options to toggle this on or off.

The wrapdb sanity check only cares about the version, not the package name, in the remote url. I reiterate: all wraps that use GitHub archives are already doing exactly this, the url uses .../<version>.tar.gz, so clearly it has to be functional as far as the wrapdb CI is concerned.

@eli-schwartz
Copy link
Member

Looking at the upstream meson.build I think a couple of improvements can be made:

  • VER_PRODUCTMAJORVERSION can probably be gotten with cc.get_define()
  • meson has a built-in option for LTO, which you can set in default_options
  • fvisibility=hidden can be set via gnu_symbol_visibility: 'hidden' and meson will use it where appropriate/supported
  • -lm does not work portably and you must not use it. On some platforms it does not exist and will result in link errors, because the necessary functions are provided in libc and there is no fake stub library: https://mesonbuild.com/howtox.html#add-math-library-lm-portably (in general you should be using cc.find_library)
  • instead of using subproject().get_variable you should just use dependency() which hooks directly into the [provide] section which nlohmann_json has.

@jfinkhaeuser
Copy link
Author

Thanks for those suggestions... I'm not sure I have the time to try them all out right now, but they are very good tips indeed. The file is based on a file I created some time ago when much of this wasn't supported.

@jfinkhaeuser
Copy link
Author

What you're describing wrt to URLs then means that wrapdb only supports GitHub, or anything that behaves the same. If there is no interest in interoperability, we might as well close this PR then.

@jfinkhaeuser
Copy link
Author

With regards to URLs: https://datatracker.ietf.org/doc/html/rfc3986#section-3.3 does not mention file names. All path components are opaque to the generic URI scheme. Even the RFC that specifically refers to the HTTP URL scheme does not mention file names; it doesn't even specify how to interpret components where the generic URI scheme does a little. https://datatracker.ietf.org/doc/html/rfc1738#section-3.3 .

This is intentional, and part of the representational nature of the REST architecture of HTTP, that even something that looks like a file path in no way has to map to a file path on some file system.

Treating the last component as a file name is at best convention, at worst a misunderstanding of URLs. Relying on that opaque component to have a syntax and meaning that's universally supported is quite optimistic.

Now wrapdb isn't my project, so you do you. I just don't have an interest in bending over backwards to support a spec that looks a bit broken.

@eli-schwartz
Copy link
Member

What you're describing wrt to URLs then means that wrapdb only supports GitHub, or anything that behaves the same. If there is no interest in interoperability, we might as well close this PR then.

I have no idea how you possibly came to that conclusion.

What I described, contradicts what you claim I described, in essentially every way possible.

Framing this as "the wrapdb is GitHub centric and only supports GitHub or github-like websites" is... frankly, it's dishonest.

[RFCs about URI/URL]
This is intentional, and part of the representational nature of the REST architecture of HTTP, that even something that looks like a file path in no way has to map to a file path on some file system.

Why are we talking about something that was invented years after the RFCs you are linking?

RFCs which define ftp:// and file:// and outright clarify that the final component is a filename.

Is now a good time to point out that the wrapdb supports ftp urls?

Relying on that opaque component to have a syntax and meaning that's universally supported is quite optimistic.

The worst part of this entire FUD is that I had already from the beginning pointed out that none of this matters -- meson wraps do not rely on this at all, because the source_filename field is required.

Nor do the lint checks in this repo require it -- the problem with your chosen software forge is that it doesn't include the version string anywhere as a substring of the entire fully qualified URI. Not even in a query string.

This is not only incompatible with GitHub, it's also incompatible with sourceforge, download.gnome.org, download.savannah.gnu.org, and numerous other ftp-like download services.

Even Oracle's download page for Java is better than this.

@eli-schwartz
Copy link
Member

I just don't have an interest in bending over backwards to support a spec that looks a bit broken.

Your erroneous and stubborn belief that this is in any way related to the URI spec is not sufficient grounds for the wrapdb lint checks to repeal the restriction that remote download urls must contain, somewhere, anywhere, the version string, in order to avoid accidents such as bumping the version of jfinkhaeuser-json-rpc but forgetting to bump the content-addressable hash and leading to the old version of the software still being used.

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.

3 participants