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

[RFC 0045] Deprecating unquoted URL syntax #45

Open
wants to merge 10 commits into
base: master
from

Conversation

@7c6f434c
Copy link
Member

commented Apr 28, 2019

No description provided.

@grahamc
Copy link
Member

left a comment

I'm in favor of this RFC.

strings or uniformly unquoted.

Decide to use unquoted URLs for all URLs without special characters or variable
expansion.

This comment has been minimized.

Copy link
@grahamc

grahamc Apr 28, 2019

Member

Mind making these two items a list?

This comment has been minimized.

Copy link
@7c6f434c

7c6f434c Apr 28, 2019

Author Member

Definitely done in the PR branch, I have no idea why GitHub doesn't show this yet…

convert URLs to quoted strings when changing them.

Accept PRs that convert unquoted URLs to quoted strings if such PRs are
submitted.

This comment has been minimized.

Copy link
@grahamc

grahamc Apr 28, 2019

Member

I wonder if there is a way ofborg could detect newly introduced URL values?

This comment has been minimized.

Copy link
@7c6f434c

7c6f434c Apr 28, 2019

Author Member

Maybe your profiling patch to Nix is in the right direction for that…

@joepie91

This comment has been minimized.

Copy link

commented Apr 28, 2019

Strongly in favour. It's never been clear to me why this was part of the syntax, and more than once I've been left with analysis paralysis because I wasn't sure whether there was a reason to use the special syntax or not, and there appeared to be no documentation on that.

And from a tooling implementor POV: removing this in the long run would reduce the complexity of a Nix (parser) implementation.

@ryantm

ryantm approved these changes Apr 28, 2019

@peti

peti approved these changes Apr 28, 2019

Copy link
Member

left a comment

Yes, please remove the special URL syntax! That feature adds very little value (if any) and I've never needed it.

@samueldr

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

Quick thoughts about deprecating language/interpreter features:

As far as detection goes, deprecating features in the interpreter could be made strict with a list of "new" features, e.g. --option strict-eval no-unquoted-urls so that the interpreter gates everything related to the unquoted URLs under conditionals, making it fail. Tracing could additionally be added at the same locations to warn, hopefully with a flag to disable tracing those.

Though, this is all at the cost of adding more conditionals, thus more possibilities for weird behaviours.

Gating it in the main interpreter with an option would also allow end-users to set it globally, allowing new deprecation-free evals to be forced on their systems, hopefully helping weed out the leftover features deprecations.

@globin

globin approved these changes Apr 28, 2019

Copy link
Member

left a comment

I agree!

Another thing with unquoted URLs is that most terminals treat the ; as part of the url making it harder to copy/click links with the special syntax.

@globin globin changed the title A small RFC on deprecating URL syntax [RFC 0045] A small RFC on deprecating URL syntax Apr 28, 2019

@globin globin changed the title [RFC 0045] A small RFC on deprecating URL syntax [RFC 0045] Deprecating unquoted URL syntax Apr 28, 2019

@7c6f434c

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2019

Thanks for the suggestion of one more reason, let's have motivation section be the longest one!

@4z3

4z3 approved these changes Apr 29, 2019

@zimbatm
Copy link
Member

left a comment

👍 happy to see this happening


Add a note in the Nixpkgs manual that the unquoted URL syntax is deprecated,
changes to Nixpkgs should not increase its use, and it is recommended to
convert URLs to quoted strings when changing them.

This comment has been minimized.

Copy link
@zimbatm

zimbatm Apr 29, 2019

Member

Is it possible to be more specific? How would everybody be notified about this new rule?

This comment has been minimized.

Copy link
@7c6f434c

7c6f434c Apr 29, 2019

Author Member
  1. I think the mere fact of an RFC passing is a noticeable effect event for a large fraction of active contributors. (It is visible in this repository, announced via Discourse, gets into the «weekly», is likely to be discussed a bit on IRC etc.)

For this specific RFC just the announcement «RFC on Deprecation of URL syntax passed» conveys enough information.

  1. It will be noted in two out of three manuals.

  2. Once we have an RFC that something «should not» happen, a request for cosmetic cleanup in a PR (and such requests happen) is more likely to contain a mention of quoting URLs (if relevant for the package in question). This also spreads the knowledge.

I do have an impression that quite a few people mentioned that they are disappointed they cannot refer to a policy that quoted URLs are better, so I expect the review channel of information dissemination to perform well.

Appendix A: future work — maybe the tool gets implemented, then we can open a countdown issue and maybe make ofborg check that PRs do not make things worse).

My plan is indeed just these points. I think that these things do not really need any additions to the text of RFC, and I think these mechanisms will be enough to distribute the information.

If you think that any of these four channels benefit from an addition to RFC text, or that there are other information distribution channels that should be used (and mentioned in the RFC text) please give some details.

This comment has been minimized.

Copy link
@zimbatm

zimbatm Apr 29, 2019

Member

It doesn't have to be formalized in the RFC but it's good to have an accompanying discussion.

For example you mentioned "they are disappointed they cannot refer to a policy". Would the RFC act as such policy? If not, maybe we should have a list of official policies in place.

This comment has been minimized.

Copy link
@7c6f434c

7c6f434c Apr 29, 2019

Author Member

I think having a deprecation notice in the manual (added in accordance with an explicit bullet point in an approved/accepted RFC) is close enough to official policy for the purpose of asking people not to do the deprecated things.

I didn't start the discussion myself, because I thought that (1) is assumed, (2) and (A) are explicitly mentioned, and (3) is a manual variant of (A) in a sense.

@Lassulus Lassulus referenced this pull request Apr 30, 2019

Merged

ssh-audit: init at 1.7.0 #60519

3 of 10 tasks complete
@marsam

marsam approved these changes May 1, 2019

Show resolved Hide resolved rfcs/0045-deprecate-url-syntax.md Outdated
Show resolved Hide resolved rfcs/0045-deprecate-url-syntax.md Outdated
start-date: 2019-04-28
author: Michael Raskin
co-authors:
related-issues:

This comment has been minimized.

This comment has been minimized.

Copy link
@7c6f434c

7c6f434c May 1, 2019

Author Member

I am not sure: the RFC template seems to imply this is for PRs implementing the RFC, not for the changes the RFC intends to undo.

FRidh and others added some commits May 1, 2019

Grammar edit: comparison of URLs, paths and strings.
Co-Authored-By: 7c6f434c <7c6f434c@mail.ru>
Style edit: possibility of future removal
Co-Authored-By: 7c6f434c <7c6f434c@mail.ru>
@7c6f434c

This comment has been minimized.

Copy link
Member Author

commented May 1, 2019

@FRidh thanks for the proofreading.

@edolstra

This comment has been minimized.

Copy link
Member

commented May 1, 2019

We should not break backwards compatibility in such a massive way. I mean, deprecation is only useful if you eventually want to remove it, at which point it becomes impossible for Nix to build old Nix expressions.

In principle, the flakes "epoch" feature would make it possible to make such language changes in a backward compatible way (e.g. unquoted URLs could be deprecated in epoch 2020). I'm not convinced it's actually useful, though. The case of x:x falls under the "just don't do that" header IMHO.

@7c6f434c

This comment has been minimized.

Copy link
Member Author

commented May 1, 2019

There are some features that we have chosen not to use in Nixpkgs anymore and we do not recommend to use these features if they can be avoided at all. Additional tooling intended for aiding Nixpkgs development can be useful without supporting such features, and easier creation of tooling is good.

The RFC definitely does not argue for actually dropping the support unless some other breaking changes happen. It will be a pity if an epoch-guarded syntax change for something major like a change in overrides happens without a cleanup of minor annnoyances.

@edolstra edolstra added the status:new label May 2, 2019

@domenkozar

This comment has been minimized.

Copy link
Member

commented May 2, 2019

In 2022 we can parse Nix identifier without first parsing a URI with a regex, which will speed up the parser.

@edolstra

This comment has been minimized.

Copy link
Member

commented May 2, 2019

@domenkozar I think URIs, identifiers etc. are all recognized using a single finite state machine, so it shouldn't matter much for performance.

@arianvp

This comment has been minimized.

Copy link

commented Jun 6, 2019

With ignore i meant. "It would be trivial to change them, without scary things happening." I wanted to assess how many urls we have that actually impact the result of derivations, and thus might break stuff

I think we should change them . However they will give a big diff if we do them all at once. Maybe we should split up that change somehow

@Infinisil

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

Here is what the diff would look like: Infinisil/nixpkgs@184fea6

This was done with https://github.com/infinisil/nix-quote-urls, so there shouldn't be any false positives or negatives.

That commit changes over half of all files in nixpkgs, not saying that's a bad thing, but there are the proportions if we want to do this.

@alexarice alexarice referenced this pull request Jun 6, 2019

Merged

sngrep: init at 1.4.6 #62769

3 of 10 tasks complete

@zimbatm zimbatm referenced this pull request Jun 25, 2019

Closed

URLs and paths #61

@mmahut mmahut referenced this pull request Jun 26, 2019

Merged

timetable: init at 1.0.6 #63788

3 of 10 tasks complete

@domenkozar domenkozar referenced this pull request Jun 27, 2019

Closed

RFC Meeting 2019-06-27 #3

@Mic92

This comment has been minimized.

Copy link

commented Jul 1, 2019

Since shepherd was not explicitly announced before. For this RFCs @edolstra is the leader, while @zimbatm and @Infinisil are the other member on the shepherd team.

@zimbatm

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Count me in.

@edolstra edolstra referenced this pull request Jul 4, 2019

Closed

RFC Meeting 2019-07-04 #4

@Mic92 Mic92 referenced this pull request Jul 4, 2019

Closed

RFC Meeting 2019-07-11 #5

@xrelkd xrelkd referenced this pull request Jul 11, 2019

Merged

tilt: 0.8.8 -> 0.9.4 #64581

4 of 10 tasks complete

@Mic92 Mic92 referenced this pull request Jul 11, 2019

Closed

RFC Meeting 2019-07-18 #6

Update rfcs/0045-deprecate-url-syntax.md
Shepherd team

Co-Authored-By: Domen Kožar <domen@enlambda.com>

@domenkozar domenkozar referenced this pull request Jul 18, 2019

Closed

RFC Meeting 2019-08-01 #7

author: Michael Raskin
co-authors:
shepherd-leader: Eelco Dolstra
shepherd-team: Eelco Dolstra, Jonas Pfenniger, Silvan Mosberger

This comment has been minimized.

Copy link
@lheckemann

lheckemann Jul 19, 2019

Member

@zimbatm is your name correct here?

This comment has been minimized.

Copy link
@zimbatm

zimbatm Jul 19, 2019

Member

no it's "zimbatm" :)

This comment has been minimized.

Copy link
@lheckemann

lheckemann Aug 15, 2019

Member

@domenkozar or @7c6f434c would you care to fix this?

This comment has been minimized.

Copy link
@7c6f434c

7c6f434c Aug 15, 2019

Author Member

Ah right, sorry.

@flokli flokli referenced this pull request Jul 20, 2019

Merged

Add spotifyd package and service #65092

3 of 10 tasks complete
@Infinisil

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

Considering that the vast majority of people are for this deprecation, which also does provide some benefits, I'm alright with it as well.

However I don't think we should actually enforce this right now, this syntax is very widely used. I'd much rather have this just be marked as deprecated (like with isNull, whose deprecation I'm not a fan of btw), so that people can be informed over time. Nixpkgs can slowly be cleaned of non-quoted urls over time as well, such that eventually we can enforce it for PR's once the majority of people already know about it. And eventually, in a long time, this can actually be removed from the Nix grammar.

This then shouldn't distract us much and we can keep focusing more important problems while dealing with this one slowly over time. How does this sound? Especially asking @edolstra

@mmahut mmahut referenced this pull request Jul 28, 2019

Merged

nim: 0.20.0 -> 0.20.2 #65421

7 of 10 tasks complete

@petabyteboy petabyteboy referenced this pull request Jul 30, 2019

Merged

hcloud: 1.11.0 -> 1.13.0 #65599

7 of 10 tasks complete

@yegortimoshenko yegortimoshenko referenced this pull request Aug 3, 2019

Merged

ledger-live-desktop: init at 1.12.0 #62753

4 of 10 tasks complete

@srhb srhb referenced this pull request Aug 5, 2019

Merged

bluejeans-gui: use patchShebangs, add me to maintainers #66091

4 of 10 tasks complete

@edolstra edolstra referenced this pull request Aug 8, 2019

Closed

RFC Meeting 2019-08-08 #8

@Mic92 Mic92 referenced this pull request Aug 15, 2019

Open

RFC Meeting 2019-08-15 #9

@zimbatm

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

Let's move forward with this. It's not too controversial but also not worth spending too much time on it. We'll amend the nix and nixpkgs manuals, maintainers and reviewers can refer to this RFC and we'll enforce it more strictly over time.

@edolstra

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

I'm not in favor of moving forward with this. It's a huge amount of work for little to no benefit.

And eventually, in a long time, this can actually be removed from the Nix grammar.

We can never do that because reproducibility requires being able to build old Nix expressions.

@zimbatm

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

What is a huge amount of work?

I think the main interest is to enforce that on nixpkgs and avoid having new users having to learn about the URL syntax. We don't need to change any of the nix implementation for this RFC to be useful.

@arianvp

This comment has been minimized.

Copy link

commented Aug 16, 2019

Can we move this forward / or not based on a consensus vote?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.