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

buildPerlPackage: phase out "name" #66453

Closed
wants to merge 3 commits into from

Conversation

@volth
Copy link
Contributor

volth commented Aug 10, 2019

Some time ago perlPackages.* have been refactored to use buildPerlPackage with pname and version instead of name
and a warning was printed when it was used with name to prompt users fix their code.
As time passed, now the warning is being replaced with a fatal error and the support for name is removed.

PS. rebuild is not expected

Some time ago `perlPackages.*` have been refactored to use `buildPerlPackage` with `pname` and `version` instead of `name`
and a warning was printed when it was used with `name` to prompt users fix their code.
As time passed, now the warning is being replaced with a fatal error and the support for `name` is removed.
@volth volth force-pushed the volth:buildPerlPackage_phase_out_name branch from 8ac60d8 to fe0d192 Aug 10, 2019
@volth volth changed the title buildPerlPackage: phase out name buildPerlPackage: phase out "name" Aug 10, 2019
@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Aug 11, 2019

I believe there was throw in stdenv.mkDerivation that did the same thing and it was reverted.
I think having the builder functions inconsistent like this would be confusing to users, for example every buildPythonPackage pretty much uses pname but it doesn't throw for name.
I think the warning is good if it only affects external expressions.

@volth

This comment has been minimized.

Copy link
Contributor Author

volth commented Aug 12, 2019

I believe there was throw in stdenv.mkDerivation that did the same thing and it was reverted.

I think stdenv.mkDerivation should print a warning right now (and maybe throw next year).

The name -> pname migration is stalled, up to new PRs get with name (well, I make them too, in the absence of warning it is easy to forget after years of using name)

The migration is important for various lang2nix and repology (#61691) because name is tricky to split to package name and version.

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Aug 12, 2019

I believe there was throw in stdenv.mkDerivation that did the same thing and it was reverted.

I think stdenv.mkDerivation should print a warning right now (and maybe throw next year).

The name -> pname migration is stalled, up to new PRs get with name (well, I make them too, in the absence of warning it is easy to forget after years of using name)

The migration is important for various lang2nix and repology (#61691) because name is tricky to split to package name and version.

Indeed all of this is true, and we'll clearly benefit from having pname and version everywhere.
So perhaps just a warning in stdenv.mkDerivation should be discussed?

@volth

This comment has been minimized.

Copy link
Contributor Author

volth commented Aug 12, 2019

So perhaps just a warning in stdenv.mkDerivation should be discussed?

Yes.
Replacing warning with throw is to be postponed till 20.03 or 20.09

@volth

This comment has been minimized.

Copy link
Contributor Author

volth commented Aug 12, 2019

Oh, a warning in stdenv.mkDerivation emits a lot of false positives
(so I doubt there has been a PR proposing throw in stdenv.mkDerivation)

warning: stdenv.mkDerivation:                       
  name = "d851fada460d42508a6f82b19867f63853062583.patch"  
is deprecated, please use                                  
  pname = "d851fada460d42508a6f82b19867f63853062583.patch";
  version = "";                                            
                                                           
warning: stdenv.mkDerivation:                       
  name = "source"                                          
is deprecated, please use                                  
  pname = "source";                                        
  version = "";                                            

warning: stdenv.mkDerivation:
  name = "daemon.conf"              
is deprecated, please use           
  pname = "daemon.conf";            
  version = "";                     

warning: stdenv.mkDerivation:
  name = "Net-DBus-1.1.0.tar.gz"    
is deprecated, please use           
  pname = "Net-DBus";               
  version = "1.1.0.tar.gz";         

warning: stdenv.mkDerivation:
  name = "unit-user-.service"       
is deprecated, please use           
  pname = "unit-user";              
  version = ".service";             

warning: stdenv.mkDerivation:                                                  
  name = "CVE-2017-6827+CVE-2017-6828+CVE-2017-6832+CVE-2017-6835+CVE-2017-6837.patch"
is deprecated, please use                                                             
  pname = "CVE";                                                                      
  version = "2017-6827+CVE-2017-6828+CVE-2017-6832+CVE-2017-6835+CVE-2017-6837.patch";

warning: stdenv.mkDerivation:      
  name = "ntfs-3g_ntfsprogs-2017.3.23.tgz"
is deprecated, please use                 
  pname = "ntfs";                         
  version = "3g_ntfsprogs-2017.3.23.tgz"; 

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Aug 12, 2019

Ah it was an assert that was reverted from the original implementation #51183

@volth volth closed this Aug 13, 2019
@volth

This comment has been minimized.

Copy link
Contributor Author

volth commented Aug 13, 2019

BTW, some language builders do not support name.

{ pname
, version ? null


{ pname, version, buildInputs ? [], meta ? { platforms = ocaml.meta.platforms or []; },

So perl won't be pioneering if pname and version will be required params of buildPerlPackage

@volth volth reopened this Aug 13, 2019
@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Aug 13, 2019

I'd say those are bugs that should be fixed before release.

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Aug 13, 2019

If we're going to do this we'd need to update all the examples at

and still people may think they could use name but at least it will give a message.

@volth

This comment has been minimized.

Copy link
Contributor Author

volth commented Aug 13, 2019

Let's keep it open and collect people opinions

@volth

This comment has been minimized.

Copy link
Contributor Author

volth commented Aug 18, 2019

On the other hand, fetchurl (which is kind of default builder for data/fonts/*) does not accept pname
That result in weird code

let
pname = "agave";
version = "10";
in fetchurl {
name = "${pname}-${version}";

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.