kasei / attean

A Perl Semantic Web Framework
19 stars 10 forks source link

Change in `import()` behaviour for Perl > 5.39.1 #168

Closed zmughal closed 3 months ago

zmughal commented 4 months ago

Fixes https://github.com/kasei/attean/issues/167.

See also https://github.com/Perl/perl5/issues/21269.

zmughal commented 4 months ago

To test this:

docker run -v $PWD:/work -w /work --rm -it perl:5.39.1 \
    bash -c '
        curl -fsSL https://raw.githubusercontent.com/skaji/cpm/main/cpm | perl - install -g App::cpm;
        cpm install -g Module::Install::AuthorTests Module::Install::DOAPChangeSets Module::Signature;
        cpanm -nq --showdeps . | xargs cpm install -g ;
        cpanm --verbose --test-only .;
        bash '
kasei commented 4 months ago

Is there an explanation for what the underlying issue here is? The linked Perl issue left things unclear to me. This particular change seems fine (since, as @kjetilk mentions, the tests still pass). But the overall trend of having to remove constructs like this seems contrary to the API design, and I'd like to understand the implications before changing code.

zmughal commented 3 months ago

Thanks for reviewing the code @kjetilk and @kasei. Yeah, I didn't fully understand the change myself. I was trying different ways of getting import() into the package… but it looks like I get it now.

After inspecting the output of Devel::TraceUse, I think that what was going on was that Attean.pm was being required by Attean/RDF.pm and this was in turn loading other packages before the Exporter::Tiny::import() was set up. So I moved it before that and changed use Attean into require Attean.

It should pass via the Docker command and the GitHub Actions (once it is re-run).

kasei commented 3 months ago

Great investigative work! And that does provide a good explanation for both the problem and why this is a good fix.