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

Improve and consolidate distribution lookup #1812

Merged
merged 32 commits into from
Dec 20, 2018
Merged

Improve and consolidate distribution lookup #1812

merged 32 commits into from
Dec 20, 2018

Conversation

ugexe
Copy link
Member

@ugexe ugexe commented May 10, 2018

Adds method candidates to CompUnit::Repository::Installation ( CURI ) and CompUnit::Repository::FileSystem ( CURFS ). All distribution searching/resolving goes through this method. This gives developers insight into how a CURI/CURFS/CUR??? resolves a module request, which was not possible with .resolve -- it would chain to the next CUR and only returned a single result. The centralization of the recommendation-manager-like functionality should aid in implementing additional NYI s22 features.

Raku/roast@cab3dd0
https://design.perl6.org/S22.html#candidates

CURI and CURFS changes

!matching-dist is now backed by .candidates, but also caches a single 'best match' so it acts similar to before.

.candidates ( and by extension !matching-dist ) now returns a Distribution object and not ($dist-id, $dist-data). This is possible because of a role that allows our lazy distribution to avoid IO calls to obtain meta data. Previously this was done by passing around a hash internally, but now we dogfood our Distribution interface throughout CURI.

Deprecates .script, and reverts to using just .files - .script was just a fast path of .files under certain conditions and lacked the ability to filter by name. Now .files can provide the faster path for other files automatically. Additionally this fixes an issue where .script would invoke older version bin/ scripts from ~/.perl6

CURFS changes

Creates full Distribution for both -I. and -Ilib, where the later will use the directory recursion it was already doing ( to determine id ) to automatically populate meta data for provides and files ( bin/, resources/ ).

Gives bin scripts a way to more robustly know their own version ( although nyi $*DISTRIBUTION would be the more appropriate way, but would still be helped by this work ):

# bin/zef
use Zef::Client;

sub MAIN("version") {
    my $spec = CompUnit::DependencySpecification.new(:short-name<Zef::Client>);
    say $*REPO.need($spec).distribution.meta<ver version>.first
}

The above currently only works for perl6 bin/zef (See: ugexe/zef#122 (comment)), and would blow up on perl6 -I. bin/zef and perl6 -Ilib bin/zef. This PR works for all 3; -I. ( which we assume is pointing at a directory with a META file ) will pull its version from the meta file, and -Ilib (a directory without a META file) is version 0.

Note this does look at ../ to build the meta data for -Ilib, but this is already the case for CURFS.resources.

ugexe added 16 commits July 30, 2017 01:16
Add `method candidates` - All distribution searching/resolving go
through this new method. This gives developers insight into how a
CURI resolves a module request, which was not possible with .resolve
(since it would chain to the next CUR, and only returned a single
result). Many NYI s22 features will require something like this, and
.candidates is indeed mentioned in s22 https://design.perl6.org/S22.html#candidates

!matching-dist is now backed by .candidates, but also caches a single
'best match' so it acts similar to before.

.candidates (i.e. !matching-dist) now returns a Distribution object
and not ($dist-id, $dist-data). This is possible because of a role
that allows our lazy distribution to avoid yet additional costly IO
calls to obtain meta data. Previously this was done by passing around
a hash internally, but now we dogfood our Distribution interface
throughout CURI.

Avoid io after setting up various directories once.

Removes .script, and reverts to using just .files - .script was just
a fast path of .files under certain conditions and lacked the ability
to filter by name. Now .files can provide the faster path for other
files automatically. Additionally this fixes an issue where .script
would invoke older version bin/ scripts from ~/.perl6

Adds :$api field wherever it was missing for consistency.
It made sense for the .files/.candidates api to take an explicit
bin/ (if looking for bin/foo). It could make sense for this
.run-script + wrapper, but it also make sense without since the
method name implies the name-path prefix is a bin/ script.

Since a rakudo installation can be upgraded with distributions
(i.e. bin/ scripts) already installed we should continue to use
the current behavior of an implicit bin/ prefix.
Even if this doesn't result in a obvious speed increase, it has
the ability to. Consider that a lazy Distribution may eventually
end up parsing json (its slow path) to fill out the rest of its
fields (anything that isn't name/auth/ver/source/checksum) - once
this happens we may as well serve all future requests with those
fields so they don't have to do another parse.
Sometimes the lookup file may not contain the source id. In these
cases we need to fall back to the slower json-parsing code path.
This also revealed a bug where old meta data was being returned
for a LazyDistribution after the first json parsing.
See #1125 for .candidates details

Creates full Distribution for both -I. and -Ilib, where the later will use the directory recursion it was already doing (to determine id) to automatically populate meta data for provides and files (bin/, resources/).

Makes it possible to do `sub MAIN("version") { $*REPO.need("My::Module").distribution.meta<ver> }` to allow scripts to give the right version regardless of CURI or CURFS being used to load it.
Fixes spectest failures from caching too aggressively.

This also returns to using a cached `$!id`
This improves the Distribution that gets created for a CURFS repo prefix that does -not- contain a META6.json file. It will guess the dist name (as the shortest module name in provides) as well as resources (trying to do a reverse platform-library-name on files in resources/libraries).

This essentially allows the following to work:
`$ perl6 -e 'my $dist = CompUnit::Repository::FileSystem.new(prefix => $*CWD.absolute).candidates("Inline::Perl5")[0]; CompUnit::RepositoryRegistry.repository-for-name("site").install($dist)'`
If a distribution is not found then do not cache the value. This allows run time module installations to be found with resolve/candidates.
For a single dist CUR like this it doesn't really matter, but following the same template as CURI allows run time generated CURFS lookup to work.
Name meta data variables $meta instead of $dist
@ugexe ugexe requested a review from niner May 10, 2018 04:28
Is easier to read, gives a nice spot for optimizations, and keeps
the -I. / -Ilib + %?RESOURCES magic kept in one place -- the
Distribution object.
@zoffixznet
Copy link
Contributor

ping is this mergeable or what is needed? It's blocking auth tests that I believe are needed to spec the auth key

@ugexe
Copy link
Member Author

ugexe commented Jul 13, 2018

Progress is blocked by a consensus ( and possible associated code changes ) for #1853 since it adds the same auth/ver/api lookup/verification to CURFS / -I. -Ilib

Additionally the performance of the CURFS changes probably need to be improved, as it slowed down the roast a fair bit ( although this appeared to be because of how poorly the roast organizes the various modules it uses/tests, resulting in having to iterate and recurse over significantly more files ).

EDIT: Actually performance is probably ok... the speed is exactly the same with TEST_JOBS=1.

Fixes a weird spectest failure in t/spec/S12-meta/exporthow.t
===SORRY!===
Type check failed in binding to parameter '$CWD'; expected Str but got IO::Path (IO::Path.new("/Users/ugexe/re...)
@ugexe
Copy link
Member Author

ugexe commented Oct 18, 2018

Perhaps the candidates method name should be renamed, since that is already used to do something completely different:

$ perl6 -e 'say &gethostname.candidates()'
(sub gethostname ( --> Str:D) { #`(Sub+{Callable[Str:D]}|140424549133624) ... })

@ugexe
Copy link
Member Author

ugexe commented Dec 19, 2018

I was going to wait until after the release was cut, but I only have the next 5 days to keep a close watch on this which is already less than I wanted. So hopefully the release can still be handled without blocking master. @AlexDaniel

@ugexe ugexe merged commit bfff01a into master Dec 20, 2018
@niner
Copy link
Collaborator

niner commented Dec 20, 2018

Yeah!

@AlexDaniel
Copy link
Contributor

@ugexe 👍 Sorry for not writing a response here, I simply didn't see the notification (getting too many of them…). Please feel free to ping me on IRC any time you need a quicker answer from me. Yes, I can easily cut a release from a branch so generally it's not an issue to commit to master (especially once we know that there's a stable revision at some point), and that's exactly what I did for 2018.12 (the release does not include cur-candidates merge, even though the release happened after the branch was merged).

Thank you for your work!

BTW there's a regression associated with this merge, though there's not much info about it currently. See https://colabti.org/irclogger/irclogger_log/perl6-dev?date=2018-12-21#l181

@AlexDaniel AlexDaniel deleted the cur-candidates branch July 9, 2019 18:01
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

Successfully merging this pull request may close these issues.

4 participants