Details

Reviewers

douardda
vlorentz

Group Reviewers

Reviewers

Commits

rDLS476177363182: cran.lister: Use cran's canonical url for origin url

Summary

Also make sure the uid is versioned (so as to allow to actually see new package
versions).

The canonical urls were seen at the available packages listing [1]

(This means an adaptation loader CRAN's side is needed)

[1] https://cran.r-project.org/web/packages/available_packages_by_date.html

Test Plan

tox

Diff Detail

Repository

rDLS Listers

Branch

master

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 10146
Build 15050: tox-on-jenkins	Jenkins
Build 15049: arc lint + arc unit

Event Timeline

ardumont created this revision.Jan 14 2020, 2:42 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DLS/job/tox/520/
See console output for more information: https://jenkins.softwareheritage.org/job/DLS/job/tox/520/console

Harbormaster failed remote builds in B10089: Diff 8989!Jan 14 2020, 2:46 PM

ardumont added inline comments.Jan 14 2020, 2:52 PM

swh/lister/cran/tests/test_lister.py
50	build failed my bad, it's a task row not a lister row.

ardumont planned changes to this revision.Jan 14 2020, 2:52 PM

Fix test

Build is green
See https://jenkins.softwareheritage.org/job/DLS/job/tox/521/ for more details.

Harbormaster completed remote builds in B10095: Diff 8995.Jan 14 2020, 4:24 PM

ardumont edited the summary of this revision. (Show Details)Jan 14 2020, 5:04 PM

Only dedicate the diff to the origin url changes

Another one will be created for the uid versioning part (~> D2531)

ardumont added a child revision: D2531: cran.lister: Version uid so we can list new package versions.Jan 15 2020, 10:53 AM

Build is green
See https://jenkins.softwareheritage.org/job/DLS/job/tox/523/ for more details.

Harbormaster completed remote builds in B10101: Diff 9000.Jan 15 2020, 10:55 AM

ardumont added a reviewer: Reviewers.Jan 15 2020, 1:15 PM

ardumont added inline comments.Jan 15 2020, 1:30 PM

swh/lister/cran/lister.py
45–47	Heads up, i will also drop that version which has no part in there. It's the artifact's version...

ardumont mentioned this in D2532: cran.loader: Align cran loader with other package loaders.Jan 15 2020, 1:47 PM

ardumont removed a child revision: D2531: cran.lister: Version uid so we can list new package versions.Jan 15 2020, 1:50 PM

Drop version as it's really the artifact's version (already defined there)

ardumont added a child revision: D2531: cran.lister: Version uid so we can list new package versions.Jan 15 2020, 1:56 PM

Build is green
See https://jenkins.softwareheritage.org/job/DLS/job/tox/526/ for more details.

Harbormaster completed remote builds in B10111: Diff 9009.Jan 15 2020, 1:58 PM

The canonical urls were seen at the available packages listing [1]

Do you mean that you consider the .../index.html links to be canonical? These pages contain the following text:

Please use the canonical form https://CRAN.R-project.org/package=<NameOfPackage> to link to this page.

Shouldn't we use that instead if we want them to be canonical?

And I don't like using an URL to an HTML file as an origin, anyway...

This revision now requires changes to proceed.Jan 15 2020, 4:15 PM

douardda added inline comments.Jan 15 2020, 4:17 PM

swh/lister/cran/lister.py
45	this html_url seems a bit strange here, and there is no mention of it in the example given in the docstring... Also it looks like this argument is in fact required, so I don't understand why it's retrieved with a kwargs.get()

Do you mean that you consider the .../index.html links to be canonical? These pages contain the following text:

In the sense we can follow a link which describes the origin, yes.

Please use the canonical form https://CRAN.R-project.org/package=<NameOfPackage> to link to this page.

nicer indeed.

And I don't like using an URL to an HTML file as an origin, anyway...

Well, yes. Currently, it's a link to a tarball so... that was better with the html page, i think.

;)

swh/lister/cran/lister.py
45	Also it looks like this argument is in fact required, so I don't understand why it's retrieved with a kwargs.get() it is. i avoided the `task_dict` method change as i'm unsure we can do it.

ardumont added inline comments.Jan 15 2020, 4:33 PM

swh/lister/cran/lister.py
45	also that's already the existing variable `version`'s case... (and policy) so... i just aligned that code... task_dict takes the model transformed earlier as parameter... so it's unfortunately tightly bound with that... (and i'm not starting yet another refactoring...) html_url is actually, in this lister's context, an artifact_url, so you do not see html_url, you see artifact_url which is present in the docstring.

Adapt according to review:

use the right canonical url
be more explicit on task_dict's parameter names

Build is green
See https://jenkins.softwareheritage.org/job/DLS/job/tox/528/ for more details.

Harbormaster completed remote builds in B10138: Diff 9036.Jan 15 2020, 4:38 PM

Shouldn't we use that instead if we want them to be canonical?

Totally btw, i've adapted the code accordingly.

I completely missed it, thanks!

ardumont added inline comments.Jan 15 2020, 5:51 PM

swh/lister/cran/lister.py
117–118	Now i'm wondering whether to change this in cran's model... (to name it appropriately...) .oO(sigh)

ardumont removed a child revision: D2531: cran.lister: Version uid so we can list new package versions.Jan 15 2020, 6:21 PM

as discussed IRL, let's go forward with this.

Ultimately, the change needed is to:

drop the R cran script
parse the listing page instead [1]
for each package found there, send the origin url [2] to the loader (as recurring task)
rewrite the loader cran so it scrapes that origin url [2] page. It then determines itself what the artifact urls it needs to ingest (taking care of T2029 even more properly than described there, task updated btw).

[1] https://cran.r-project.org/web/packages/available_packages_by_date.html

[2] https://cran.r-project.org/package=<package-name>

ardumont mentioned this in rDLDBASEaeac846cd13f: cran.loader: Align cran loader with other package loaders.Jan 16 2020, 12:25 PM

vlorentz accepted this revision.Jan 16 2020, 1:42 PM

This revision is now accepted and ready to land.Jan 16 2020, 1:42 PM

Rebase and plug to master branch

Build is green
See https://jenkins.softwareheritage.org/job/DLS/job/tox/531/ for more details.

Harbormaster completed remote builds in B10146: Diff 9040.Jan 16 2020, 1:55 PM

Closed by commit rDLS476177363182: cran.lister: Use cran's canonical url for origin url (authored by ardumont). · Explain WhyJan 16 2020, 1:55 PM

This revision was automatically updated to reflect the committed changes.

ardumont added a commit: rDLS476177363182: cran.lister: Use cran's canonical url for origin url.

ardumont mentioned this in T2029: cran lister: Align lister to output list of tarballs per origin.Jan 17 2020, 12:35 PM