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

Branch not being imported due to deprecated license identifier #858

Closed
kdambekalns opened this issue Jan 18, 2018 · 22 comments
Closed

Branch not being imported due to deprecated license identifier #858

kdambekalns opened this issue Jan 18, 2018 · 22 comments

Comments

@kdambekalns
Copy link

Wondering why a new branch does not show up, I manually updated on packagist.org and see this afterwards:

Reading composer.json of neos/neos-development-collection (update-gedmo-doctrine-extensions)
Importing branch update-gedmo-doctrine-extensions (dev-update-gedmo-doctrine-extensions)
Skipped branch update-gedmo-doctrine-extensions, Invalid package information: 
License "GPL-3.0+" is a deprecated SPDX license identifier, use "GPL-3.0-or-later" instead

Deprecated does not mean illegal, but it seems this breaks package visibility on packagist for me.

Any hints? Information? Puh-lease… :)

@jeabakker
Copy link

I have the same issue with one (or all) of our packages

@ryanaslett
Copy link

ryanaslett commented Jan 18, 2018

Also received an email from packagist stating:

The drupal/drupal package of which you are a maintainer has
failed to update due to invalid data contained in your composer.json.
Please address this as soon as possible since the package stopped updating.

You probably should not simultaneously depend upon idempotent metadata that is stored in git commits, and also deprecate (or in this case mark as invalid) uses of that data. Its essentially an API break.

in our case we're seeing the following:

Reading composer.json of drupal/drupal (8.0.x)
Importing branch 8.0.x (8.0.x-dev)
Skipped branch 8.0.x, Invalid package information:
License "GPL-2.0+" is a deprecated SPDX license identifier, use "GPL-2.0-or-later" instead

Reading composer.json of drupal/drupal (8.1.x)
Importing branch 8.1.x (8.1.x-dev)
Skipped branch 8.1.x, Invalid package information:
License "GPL-2.0+" is a deprecated SPDX license identifier, use "GPL-2.0-or-later" instead

Reading composer.json of drupal/drupal (8.2.x)
Importing branch 8.2.x (8.2.x-dev)
Skipped branch 8.2.x, Invalid package information:
License "GPL-2.0+" is a deprecated SPDX license identifier, use "GPL-2.0-or-later" instead

Reading composer.json of drupal/drupal (8.3.x)
Importing branch 8.3.x (8.3.x-dev)
Skipped branch 8.3.x, Invalid package information:
License "GPL-2.0+" is a deprecated SPDX license identifier, use "GPL-2.0-or-later" instead

Reading composer.json of drupal/drupal (8.4.x)
Importing branch 8.4.x (8.4.x-dev)
Skipped branch 8.4.x, Invalid package information:
License "GPL-2.0+" is a deprecated SPDX license identifier, use "GPL-2.0-or-later" instead

Reading composer.json of drupal/drupal (8.5.x)
Importing branch 8.5.x (8.5.x-dev)
Skipped branch 8.5.x, Invalid package information:
License "GPL-2.0+" is a deprecated SPDX license identifier, use "GPL-2.0-or-later" instead

Reading composer.json of drupal/drupal (8.6.x)
Importing branch 8.6.x (8.6.x-dev)
Skipped branch 8.6.x, Invalid package information:
License "GPL-2.0+" is a deprecated SPDX license identifier, use "GPL-2.0-or-later" instead

We will not be making changes to the 8.0.x -> 8.3.x branch, as those branches are closed now, so Im not sure how we're going to be able to proceed here.

I suggest that this evolve into a warning that does not prevent further processing, and still supports "GPL-2.0+" as a valid option, albeit with a message stating that it should use the more correct license label.

@legoktm
Copy link

legoktm commented Jan 18, 2018

This wasn't intentional, it was just supposed to be a warning, not an error. Let me see if I can figure out why this is happening...

@legoktm
Copy link

legoktm commented Jan 18, 2018

https://github.com/composer/composer/blob/015927d0b058eb5d31e098952f7f1ab9613cb959/src/Composer/Repository/VcsRepository.php#L249-L265

VcsRepository in composer throws an exception about having warnings, which it then catches, which sets $this->branchErrorOccurred = true;. Packagist checks $repository->hadInvalidBranches() (returns $this->branchErrorOccurred) which seems sensible, and fails the update.

Maybe validation warnings without errors should not set $this->branchErrorOccurred = true;?

@andrerom
Copy link

Having this issue as well, fixing all repos will be exhausting, fixing all branches of all repos...

@naderman
Copy link
Member

Branches are moving targets that can be modified. That's why this doesn't throw an error on tags which cannot be modified. Please update your branches.

@emodric
Copy link

emodric commented Jan 20, 2018

As it was mentioned before, some projects close the obsolete branches and do not update them any more. Sometimes it's even company policy. And as @andrerom mentioned, updating ALL branches of ALL repos will be exhausting and error prone, since it probably can't be automated due to various release policies for various repos.

@lserwatka
Copy link

lserwatka commented Jan 20, 2018

In our open source project we have over 45 packages submitted to packagist.org. That will result in hundreds of branches to update. Nearly impossible to make such a cleanup in a proper way. Please restore previous behaviour.

@kdambekalns
Copy link
Author

Ok, so ot's not as bad, since AFAICT only branches unknown yet will be ignored due to this. So existing branches do not have to be updated (right now).

Still, when not going through the Packagist website to update (and who does, a GH hook is way more comfortable), you'll get no hint whatsoever.

So, again: tell me where to doucment and I'll amend Composer docs. And I'd be happy if that was somehow announce on the Packagist site and/or notifications would be made in case of this happening.

@weitzman
Copy link

There are way too many old branches around in my repo to make this an easy process. Please please downgrade to a warning.

@andrerom
Copy link

andrerom commented Jan 21, 2018

@naderman I agree we should upgrade our branches, with time, but:

  1. It's supposed to be a warning, so that it becomes blocking is a bug, so please tell us how we can help fix it
  2. I don't really get the original issue here, for me for instance GPL-2.0 means just GPL 2.0, there is no any later version implied in that. So good that some one wants to be this super explicit, but to be frank, making a blocker (exception) out of this is a bit to strict.

So again, how can we help improve packagist to not throw on composer warnings?

@Seldaek
Copy link
Member

Seldaek commented Jan 21, 2018

Alright sorry everyone.. I was hoping this would push all projects to move to the new identifiers quickly but I realize this was probably too hopeful given the amount of projects involved and the fact that as you say many historical branches are to be fixed and that blocks updates.

I tweaked the code in composer/composer@5a1765c so that only new/updated branches will fail the validation and need to be updated. Old branches will be treated as valid even if they use deprecated license identifiers.

I hope that's a compromise everyone can live with.

@Seldaek
Copy link
Member

Seldaek commented Jan 21, 2018

Also please note that you can use composer config license and composer config license <newIdentifier> to retrieve and set the license programmatically. If you have a lot of projects to update that might help.

@jeabakker
Copy link

@Seldaek Trying to move people to use a better supported feature isn't a bad thing. However how are we (the community) to know that this change is coming?

There is (as far as i know) no blog with upcoming changes / new features. The documentation of the license part isn't inline with the change https://getcomposer.org/doc/04-schema.md#license

The only thing i found was the release notes https://github.com/composer/composer/releases/tag/1.6.0 which just mentioned that the old licenses were deprecated.

@Seldaek
Copy link
Member

Seldaek commented Jan 22, 2018

I updated the license docs, thanks for pointing that out. There is no blog of upcoming changes no, we do what we can in terms of communication, you know this being an open source project and all. I do work hard on keeping the changelog readable and informative by going through all commits before tagging a release and writing something by hand instead of dumping a list of commit messages into it, but that's all I manage to do apparently.

@andrerom
Copy link

andrerom commented Jan 22, 2018

@Seldaek Think this will help a bit, thanks.

BTW will the new identifiers work on older composer versions (silently or with warning), or will it break down?

@Seldaek
Copy link
Member

Seldaek commented Jan 22, 2018

Older versions will install these packages just fine as we don't validate anything when installing. If you were to run composer validate on a project using e.g. GPL-3.0-only with an old Composer version then it will complain that it's invalid but you can ignore that. Packagist is the only thing enforcing anything, and that can't really be outdated.

@andrerom
Copy link

Ok, good to know. Note that for future changes like this (and ideally including this one), it would be greatly appreciated if Composer had such depreciations until next major (SemVer style) before breaking on them, even for new branches. Same goes for packagist, given it's from user perspective a part of the tool.

Being pushed to adapt "non essential code" and spend time on fixing that across lots of packages is annoying to say it mildly, looking at you phpUnit 4 -> 5 -> 6 -> 7 👀 ...

rutiolma added a commit to openfed/openfed8 that referenced this issue Jan 23, 2018
rutiolma added a commit to openfed/openfed8 that referenced this issue Jan 23, 2018
@ryanaslett
Copy link

This was still an API break of sorts. Changing the definition of whats valid for a field mid-stream, and not having it act as a deprecation, but instead as an error, forces us to make these changes immediately if we want our data on packagist.org to continue to be updated. It forced us to file a critical issue to restore functionality. I just wish that packagist.org could have put up a deprecation warning instead of failing on what was previously valid data.

@andrerom
Copy link

Yes @ryanaslett, somewhat same I was asking for above. More predicable breaking changes, not pushing it down peoples throats over night.

I'm getting emails for all new PR's employees create now on our repos saying packagist failed...
(´・ω・`)

@Seldaek
Copy link
Member

Seldaek commented Jan 23, 2018

Yes, I think I got your point. Duly noted for next time. IMO we are pretty good with BC overall and rarely break anything. In this case this wasn't even a change we wanted to do but it was introduced by a third party (SPDX). Anyway I hope we can move on here. If you update all your master branches (which as I mentioned above should be somewhat easy to script if you have a lot of repos) then PRs will not fail anymore?

PR branches should also not be on the repo itself but on forked repos IMO but I don't wanna be pushing my workflow choices down your throat on top of it ;)

@andrerom
Copy link

andrerom commented Feb 28, 2018

@Seldaek: Even after spending notable time and fixing this in most cases, packagist is still spamming us with false errors on SPIX being wrong on older branches which is not receiving updates any-longer..

ref:

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

No branches or pull requests

10 participants