Remove dependency on Net::IP, and use Net::IP::XS everywhere #1119
Conversation
|
|
|
@matsduf |
If this is to be the switch, all places should be handled. I have created a pull request against your develop branch for the file, to be included in this PR. In Are there more places? I do not think so, but I can be wrong. |
Removes Net::IP as dependency, use Net::IP::XS only.
Replace references to Net::IP with references to Net::IP::XS
|
Hi @matsduf I believe all the code changes are done now. There is only one unresolved discussion, about the Dockerfile. I do not believe it is necessary to explicitly install Net::IP::XS, because it will be pulled in as a dependency of the engine. If you disagree, I would like to understand why. |
|
As you can see all direct dependencies are listed in Dockerfile, either as binary package or as CPAN package. In that way there is a complete list. If Net::IP::XS is not included, then it would be inconsistent since the other dependencies of CPAN are listed there. |
|
I made a mistake in my pull request to your pull request. I have added another commit to fix that. I think you have to accept to make it to be included. |
Adds missing variable setting
| } | ||
|
|
||
| 1; | ||
|
|
||
| =head1 NAME | ||
|
|
||
| Zonemaster::Engine::Net::IP - Net::IP/Net::IP::XS Wrapper (STILL EXPERIMENTAL) | ||
| Zonemaster::Engine::Net::IP - Net::IP::XS Wrapper |
There was a problem hiding this comment.
We should probably mark this module as deprecated in this release. In a future release we can drop it entirely and just use Net::IP::XS directly instead.
There was a problem hiding this comment.
We should probably mark this module as deprecated in this release. In a future release we can drop it entirely and just use Net::IP::XS directly instead.
If Net::IP is not dropped then #1108 should be merged to work-around the bug in Net::IP.
|
The issue has been resolved by commit 72d9b12. |
Changes loading of Net::IP::XS
|
@mattias-p, @blacksponge and @PNAX, can you please review? The PR passes the unit tests now. |
| my $p_class = eval { | ||
| require Net::IP::XS; | ||
| return q{Net::IP::XS}; | ||
| } || eval { | ||
| require Net::IP; | ||
| return q{Net::IP}; | ||
| }; | ||
|
|
||
| if ( $p_class ) { | ||
| $p_class->import; | ||
| } | ||
| else { | ||
| die "Both Net::IP and Net::IP::XS missing?\n"; | ||
| die "Net::IP::XS is not loaded\n"; | ||
| } |
There was a problem hiding this comment.
2 questions here:
- why can't we just have
use Net::IP::XShere? I've tested it and tests are failing, it seems we can't import the module at compile time, any idea why? - can we get rid of the
$p_classvariable with something like:eval { require Net::IP::XS; Net::IP::XS->import; }; if ( $@ ) { die "Net::IP::XS is not loaded\n"; } Net::IP::XS->new( '192.0.2.1' );
There was a problem hiding this comment.
- I very much believe we could.
- A simple
use Net::IP::XSwould replace all of this code too.
The reason we had all this complexity in the first place was to make things work smoothly even if either one of Net::IP::XS or Net::IP was unavailable. Since we're dropping the dependency on Net::IP and promoting Net::IP::XS to a hard requirement we can just use that the normal way we don't need to do anything fancy at all.
There was a problem hiding this comment.
On the other hand I'm not sure it's right to demand of @anandb-ripencc to do all the clean up. In my view we shouldn't be so hard on external contributors. I think fine if we the team clean things up to our own standards after merging an externally contributed PR that's functionally correct.
There was a problem hiding this comment.
Oh, and the clean up should probably be to remove the entire Zonemaster::Engine::Net::IP module, and replace all its uses with direct calls into Net::IP::XS.
There was a problem hiding this comment.
In my first iteration I had
use Net::IP::XS;
my $p_class = q{Net::IP::XS};
but then many unit tests failed (see #1121).
$p_class is used in other places in the code.
The update in this PR is the minimal change to make it work with Net::IP::XS without Net::IP. If this PR is OK, then a clean-up could be done later.
There was a problem hiding this comment.
Oh, and the clean up should probably be to remove the entire Zonemaster::Engine::Net::IP module, and replace all its uses with direct calls into Net::IP::XS.
I suggest that we take things in steps. Such a changes also affects Zonemaster-CLI and Zonemaster-Backend.
|
@mattias-p and @PNAX, are you fine with the PR as-is? If so, please approve. The change in |
There was a problem hiding this comment.
On the other hand I'm not sure it's right to demand of @anandb-ripencc to do all the clean up. In my view we shouldn't be so hard on external contributors. I think fine if we the team clean things up to our own standards after merging an externally contributed PR that's functionally correct.
You have a very good point. Thanks @anandb-ripencc for providing this PR!
|
@mattias-p and @blacksponge, can you review too? Since I have worked on this PR I want another approval before I merge. |
matsduf
added
the
V-Patch
Purpose
This PR replaces Net::IP with Net::IP::XS. It doesn't get rid of old code that still checks for Net::IP. That can come in a later PR that does cleanup and also updates the installation documentation.
Context
This replaces #1113 and #1114 and #1108
Resolves #1107
Changes
...
How to test this PR
...