Conversation
|
@Mindavi note that setting |
doc/stdenv/meta.chapter.md
Outdated
There was a problem hiding this comment.
Not every repository is HTTP(S)-browsable.
Indeed FTPs were a very common thing some short time ago, and there are projects of hosting forges on I2P networks.
Further, automatically filling this attribute is not a good idea. It should be explicitly set when desired.
| HTTP-browsable source tree of the package. Automatically set if the package uses a `fetchFrom*` fetcher for its `src`. Example: `https://github.com/forthy42/gforth` | |
| Source repository of the package. | |
| Examples: | |
| - `https://git.savannah.gnu.org/git/gforth.git` | |
| - `https://github.com/NixOS/nixops` | |
| - `https://nest.pijul.com/pijul/pijul` |
There was a problem hiding this comment.
Not every repository is HTTP(S)-browsable.
no, but meta.repository is specifically for http-browsable source trees. it exists for maintainer convenience and to be displayed by sites like search.nixos.org
There was a problem hiding this comment.
Why only HTTP-browsable repositories? Why such discrimination?
There was a problem hiding this comment.
because this field is meant to be presented via a web browser.
would you accept a gemini or gopher link as a value for meta.homepage?
There was a problem hiding this comment.
Yes, I would.
Not a long time ago browsers displayed Gopher and even FTP. They dropped this because of code base cleanup.
What will be next? Dropping non-conformant forges like viewcvs?
Further, with some hack modern web browsers can rely on extensions that allow looking at those sites.
Further, special purpose clients still exist for them.
Further, Emacs has support for them:
There was a problem hiding this comment.
@AndersonTorres could you elaborate on this?
Further, automatically filling this attribute is not a good idea. It should be explicitly set when desired.
I think this actually a great idea! When fetching from a repo, linking to that exact repo seems to be the sane thing to do, and exactly what I would expect as a user. Of course there are cases where that isn't possible (if you have multiple sources, when fetching from an unknown forge that we don't know how to translate, etc.), but those are the only ones that should be handled manually, IMO.
The other option of adding the new meta field to every single package manually results in additional (and unnecessary) maintainer load.
There was a problem hiding this comment.
because without it, ofborg-eval fails due to niche consequences of lazy evaluation.
basically, evaluating meta now evaluates src, which causes problems for this package specifically.
There was a problem hiding this comment.
Rewording it, your meta.eyecandy field broke Nixpkgs.
There was a problem hiding this comment.
@AndersonTorres If you want your criticism to be meaningful I'd suggest not leaving it up for interpretation.
There was a problem hiding this comment.
How exactly is this open to interpretation? Asking sincerely.
Creating a meta.XYZ field introduced a breaking change.
A breaking change in an otherwise completely unrelated piece of code.
How can this possibly differ from
Rewording it, your meta.eyecandy field broke Nixpkgs.
?
There was a problem hiding this comment.
I don't consider useful metadata to be eye candy, that's why I'm asking for clarification.
How that relates to the field's relationship to the derivations src attribute is the actual question we should be following up.
There was a problem hiding this comment.
I don't consider useful metadata to be eye candy, that's why I'm asking for clarification.
Neither me. Indeed Aleksanaa coninced me of a similar idea.
However the arguments from this PR's creator about which values are acceptable revolve around eyecandy.
The usefulness of this new field, according to themm, is basically "only HTTP-browsable values are allowed because I want to see them on my web browser that supports only HTTP".
pkgs/stdenv/generic/check-meta.nix
Outdated
There was a problem hiding this comment.
As I said before, we should not rely on setting this automatically.
Since you and @Aleksanaa are arguing for including this, it should be explicitly set by the package maintainer, never inferred.
There was a problem hiding this comment.
Further it should be a list of repositories, since it is very common a repo having backup mirrors.
There was a problem hiding this comment.
it is clear that you are misunderstanding the point of this field.
it is not for use by update scripts, that is what src.gitRepoUrl is for.
As I said before, we should not rely on setting this automatically.
you say this, but you do not specify why.
please give an example of when setting this automatically would lead to undesired behavior. several (most?) of the meta.* fields are set automatically.
There was a problem hiding this comment.
NB packagers would have to explicitly set it to null if there's no browsable repo
There was a problem hiding this comment.
if there's no browsable repo, src.meta.homepage will not be set, and thus meta.repository will not be set either.
There was a problem hiding this comment.
it is clear that you are misunderstanding the point of this field.
It is clear you gave no reason why this field should point to an HTTP-browsable site to begin with.
it is not for use by update scripts, that is what
src.gitRepoUrlis for.
This is irrelevant.
First, nothing hinders this field to be useful for other applications besides search.nixos.org. Second, nothing should hinder this field to be used by other applications besides search.nixos.org.
Third, what is the relevance? When I said this should be made for update scripts?
I merely suggested this could be a multi-valued field, since this is a typical occurrence in nature.
As I said before, we should not rely on setting this automatically.
you say this, but you do not specify why.
please give an example of when setting this automatically would lead to undesired behavior. several (most?) of the
meta.*fields are set automatically.
Behold!
There was a problem hiding this comment.
It is clear you gave no reason why this field should point to an HTTP-browsable site to begin with.
it is supposed to be presented to the user via sites like search.nixos.org
therefore it should be a link that can be opened by a browser
This is irrelevant.
the existance of a similar but semantically different field is extremely relevant.
there is no reason for this field to represent a machine-readable git repository, as there is already another field that represents that.
First, nothing hinders this field to be useful for other applications besides search.nixos.org.
Second, nothing should hinder this field to be used by other applications besides search.nixos.org.
yes, that is correct. any other application that wants a human-readable, HTTP-browseable source code repository can use this field, because that is what this field represents.
Third, what is the relevance? When I said this should be made for update scripts?
yes, that is exactly the relevance
I merely suggested this could be a multi-valued field, since this is a typical occurrence in nature.
yes, while mirror repositories are common, i don't think modeling that is worth the added complexity.
- most fetchers already handle mirrors in some way
- having the user pick from a list of mirrors may cause confusion, needlessly clutter the UI, and make using the value more difficult
if you can show me a practical usecase where multiple values would be helpful, i can update this to allow multiple values.
There was a problem hiding this comment.
some packages have multiple sources, like trellis, and thus multiple repositories.
mweinelt
left a comment
There was a problem hiding this comment.
Chances are, that with the introduction of repository, the homepage field will lead to more end-user facing documentation, which is totally fine.
But if the repository URL is now auto-computed, that means there is no actual link in the package definition to follow, which is a downgrade for the development experience.
Note that this will be the third URL in meta: homepage, downloadPage and now repository. I wonder specifically about the redundancy between downloadPage and repository.
|
Honestly, between |
The use cases I personally have are:
None of these entail any interest in contributing any code upstream, nor is it even git-specific (!) as some of the other discussion implied - projects using hg, fossil, etc. often have browsable repos as well. Secondly, when I was new to nixpkgs, I found it fairly confusing to click "Source" on the search page and end up in nixpkgs. I guess this is how other distros (Arch, Debian) also work, it's not as if nixpkgs is unique in this regard, but it's not intuitive to necessarily show packaging concerns on the search page. And if we are doing so already, then I don't see the scope creep of showing yet another useful link with relevance to packaging (and disambiguating them properly, e.g. "nixpkgs source" vs "upstream source" or such).
I can see that, but I'd think that preferable to a bunch of PR churn (thinking back to
Mirrors exist to at least increase availability; if one repo is down, I'd still like to be able to view the source. |
we could set a guideline that at least one of
|
this exact disambiguation is one of the reasons i didn't name it something like i think if you just put an "upstream repository" link next to the links in the search results that would be fine, but i suppose that's a problem for another time.
that's certainly a nice thought in theory, but here's what would be needed to be able to use it in practice:
is it worth the effort to do all that just so you don't have to use the wayback machine or search "PACKAGE_NAME source code mirror"? it would be a fairly simple change to make check-meta allow multiple values, but i'm mostly concerned about the additional work that it will require in search.nixos.org (mainly in figuring out how to display multiple repos) |
First, this is a backwards thinking: Nixpkgs can exist without the search engine, the search engine can't exist without Nixpkgs. Second, what is the problem in providing a pointer to a repository that is not HTTP-reachable? Why the search engine can't return non-HTTP links as pieces of info? There is a truckload of non-HTTP-links info already, such as licenses, descriptions, platforms... Or should we provide an OSI link for each license and another for each broken platform?
Can you follow the context of what I wrote? "This is irrelevant" is explained right below: being useful to automation scripts is not a demerit for a pointer to a repository.
No, there is not. Relying on Curiously Nixpkgs was not affected because our fetchers do not deal with the archives but rather with their contents. Hell, we have in-house examples!
The relevance of
You can easily replicate the And, complexity? You are complaining about complexity, when you are writing such a complicated code that it broke an otherwise completely unrelated code?
Why are you bringing the fetchers? Just because your
Oh boi.... You yourself employed an unofficial mirror to GForth! https://github.com/forthy42/gforth
Or what? Savannah is not eyecandy enough when compared to GitHub's bells and whistles for mentally-overloaded programmers? |
|
stop saying "eyecandy" i don't care about appearances. you keep telling me what i want, and i'm tired of it. |
|
I want to pick some ideas tomorrow.
|
i can try to make |
|
I said nothing about removing fields. (Which is a bit ironic!) But this is irrelevant - you are not convinced about defining |
|
ok so instead of |
|
|
ok, but as i understand it, you want to introduce it as a new field that will have a value identical to |
Yes, and it is a chicken-egg problem. |
|
can we just add the field in it's current state then come back and add |
|
Yes, but Can we just make the field a list, like in After all you did not answered the arguments about making it a list of URLs, even after still using a MIRROR of GFORTH in your example. Ah! Another reason for keeping mirrors: SLAPPers and preservation of abandonware! |
pkgs/stdenv/generic/check-meta.nix
Outdated
There was a problem hiding this comment.
Hum, another suggestion:
Since you are grabbing src.meta.homepage regardless its existence (and dealing with the non-existence gracefully), please grab the yet-non-existent src.meta.repository too.
There was a problem hiding this comment.
If you want a different implementation, please open another PR yourself. The author's purpose is already very clear.
There was a problem hiding this comment.
This is not a different implementation, just an add-on. Have you read the "too" in the end?
There was a problem hiding this comment.
well, it's merged now, so you are free to open your own PR that builds on top of it.
There was a problem hiding this comment.
I don't wanna lose my vacation days.
(You know what?...)
#300286
00f8f07 to
7e1443a
Compare
|
@a-n-n-a-l-e-e squashed! |
ghost
left a comment
There was a problem hiding this comment.
built stdenv & tests on aarch64/x64 darwin
|
The implementation in this PR suffers from some performance issues. To future nixpkgs mergers: |
stdenv/check-meta: Fix performance regressions introduced in #294347
looked at the performance and it was less than 0.2 % increase in cpu time. however, other eval runs reported different increases with the same change which seems that the perf result for the change is lost in the noise. additionally, it doesn't appear that #300177 has any effect when compared to no-op changes like r-ryantm updates. |
https://hydra.nixos.org/build/254689522/nixlog/1/tail |
shouldn't a binary-only package like that just lack the the code should check if also, a specific example would be appreciated so i can replicate the issue. |
|
the brscan4 failure seems to be exclusive to aarch64, since |
|
It is specific to platforms where it's not supported, because that's where it throws, yes. |
|
what do people think about just wrapping this in tryEval or deepSeq so this doesn't happen? |
|
hmm problem is tryEval doesn't catch index errors (and idk why i thought deepseq would do anything) |
|
alternatively, we could make it default to nil or an empty list, and remove the optional and |
|
Some useful ideas on catching throws can be found on this nice code from @matejc (in memerian from Nixpkgs): |
|
tryEval doesn't catch the kind of error that is causing eval to fail. try i opted instead to just try to avoid the error. |
|
|
|
we already use the |
|
these packages may as well be setting |
|
Aborts, contrary to throws, are not catch-able - this needs another thing... |
| then | ||
| [ attrs.src ] | ||
| else | ||
| lib.filter (src: src ? meta.homepage) attrs.srcs; |
There was a problem hiding this comment.
i just noticed something while testing a rewrite: this filter statement needs to be moved outside the if, because otherwise the case of attrs.src not having a meta attribute is not handled.
There was a problem hiding this comment.
fixing this in the newest version

Description of changes
closes #293838
the new
meta.repositorywill, by default (i.e. unless it is specified manually by the package), pull its value fromsrc.meta.homepage, which is set byfetchFromGitHuband otherfetchFrom*functions. if this cannot be found, it will be set to null.rationale for name: chosen to make sense if added to search.nixos.org. meta.sourceCode or meta.sourceTree would make sense if not for the fact that search results already have a "Source" field which points to the location of the package.nix file in nixpkgs.
Things done
stdenvandrustPlatform.buildRustPackagefetchzipnix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.