Give more freedom when configuring Zonemaster::LDNS #134

ghost · 2022-03-01T13:16:09Z

Purpose

Make it possible to easily link OpenSSL when they are installed in non common locations.

Context

Addresses #129, #96

Changes

Add new optional features --openssl-inc and --openssl-lib.
In a way this reinstates the removal performed in #44.

And document optional features within Makefile.PL.

Also update t/rr.t to fix tests relying on the network since the zones have evolved.

How to test this PR

Installing Zonemaster::LDNS using custom folder for OpenSSL include and library should work.

cpanm -v --configure-args="--openssl-inc=/tmp/include/openssl --openssl-lib=/tmp/lib/openssl " Zonemaster-LDNS-2.2.1.tar.gz

Note

On CentOS 7, this is quite hacky.

Either install openssl-devel to have some headers in common location (where the LDNS configure script will look for, but unused at compile time).

Or create a file in PATH/include/openssl/ssl.h to delude LDNS configuration script, and add the --prefix-openssl option:

touch /tmp/openssl/include/openssl/ssl.h
cpanm -v --configure-args="--prefix-openssl=/tmp/openssl --openssl-inc=/tmp/include/openssl --openssl-lib=/tmp/lib/openssl " Zonemaster-LDNS-2.2.1.tar.gz

mattias-p

I haven't tested but it looks very reasonable. I have a few suggestions on wordings and a question about behavior.

Makefile.PL

mattias-p · 2022-03-01T14:34:19Z

Makefile.PL

+        print "WARNING 'openssl-inc' option overridden by 'prefix-openssl'\n" if ( $opt_openssl_inc );
+        print "WARNING 'openssl-lib' option overridden by 'prefix-openssl'\n" if ( $opt_openssl_lib );


Wouldn't it be better to simply die here?

Allow passing distinct OpenSSL paths for include and library files when configuring LDNS.

The `se` and `nic.se` zones have evolved a little. The tests made over the network using these zones have been slightly updated to fix errors and be aligned with current zone configuration.

ghost · 2022-03-03T19:01:13Z

With the discussion on the mailing list and the insights given, I chose to:

relax the logic when using --openssl-lib and --openssl-inc
remove the logic related to libidn for now (I need to perform more tests, I think this can be part of another PR)

matsduf

README.md should also be updated. It contains installation instructions.

matsduf · 2022-03-25T15:30:48Z

Makefile.PL

+Enable (or disable) support for converting domain names from unicode to A-label
+format.


Enable (or disable) support for converting IDN labels in U-label format (with non-ASCII Unicode characters) to the same IDN labels in A-label format (encoded in ASCII).

matsduf · 2022-03-25T15:32:31Z

Makefile.PL

+=item --[no-]randomize
+
+Experimental.
+Randomizes the capitalization of returned domain names.
+Enabled by default.


Disabled by default. -- I think this feature should be removed.

good catch, fixed

ghost · 2022-04-07T10:10:53Z

README.md should also be updated.

Updated. Please re-review.

matsduf

The change in t/rr.t is not documented, and it is unrelated to the topic of the PR.

The best solution is to break it out from this PR and present it in a PR of its own with a description it is about.

The second best solution is include a clear description of the change.

matsduf · 2022-04-08T11:34:25Z

README.md

@@ -173,7 +173,8 @@ Randomizes the capitalization of returned domain names.
 ### Custom OpenSSL

 Disabled by default.
-Enabled with `--prefix-openssl=/path/to/openssl`.
+Enabled with `--prefix-openssl=/path/to/openssl` or
+`--openssl-inc=/path/to/openssl_inc` or `--openssl-lib=/path/to/openssl_lib`


Should it be "and" instead of "or" on line 177 (updated text) to match the statement on line 190-191 (updated text):

same parent directory, use `--openssl-inc` and `--openssl-lib` options to specify both paths.

I didn't want to restrict the usage of the options. One could use them separately. Hence the "or" line 177. About the documentation line 190-191, I think the meaning is also correct. You need both options to "specify both paths", hence the "and". Are you saying that this is confusing?

I think the text in 189-191 says that both should be set if not headers and libraries are in the same directory.

Which is how I expect people to use such options. But I again I didn't want to restrict the usage. The line 177 is the documentation of the following line in Makefile.PL:

zonemaster-ldns/Makefile.PL

Line 97 in f39abb9

my $custom_openssl = ( $opt_prefix_openssl or $opt_openssl_inc or $opt_openssl_lib );

I think you say "and" above but "or" in the documentation.

You have added a lot of POD docuementation into Makefile.PL, which we do not have in other repositories. The documentation overlaps with the documentation in the README.md file. Wouldn't it be better to put the documentation in one place and then refer from the other?

which we do not have in other repositories

Looking at other repositories, I don't think we provide arguments to pass options to Makefile.PL.

Wouldn't it be better to put the documentation in one place and then refer from the other?

I guess this would be better indeed. I'd say it would be better to have it close to the code so that you can refer to it easily without needing several files. And you can run perldoc Makefile.PL to access the documentation.

I think it would be fine to state "run perldoc Makefile.PL" in README.md.

I'm not sure about removing the documentation from the README. And on another hand I think it's nice to provide POD for the Makefile.PL file. Can't we try to keep both documentation for now?

If there are no conflicts between them there is at least no short-term harm. So that is OK.

ghost · 2022-04-11T09:42:54Z

The change in t/rr.t is not documented, and it is unrelated to the topic of the PR.

This change is needed to make the tests pass (because Travis runs the test with live data).

The best solution is to break it out from this PR and present it in a PR of its own with a description it is about.

If we do so, tests will fail, and we'll need to first review, approve and merge the other PR before going further with this one. I'd rather avoid this extra work.

The second best solution is include a clear description of the change.

The change has been documented in the commit message: 9e46c8d

matsduf · 2022-04-11T10:07:44Z

The best solution is to break it out from this PR and present it in a PR of its own with a description it is about.

If we do so, tests will fail, and we'll need to first review, approve and merge the other PR before going further with this one. I'd rather avoid this extra work.

It will make it easier to track the changes and it is more transparent.

The second best solution is include a clear description of the change.

The change has been documented in the commit message: 9e46c8d

That is not enough. Please update the description plus the header of this PR to make it transparent.

ghost requested review from mattias-p and matsduf March 1, 2022 13:16

mattias-p reviewed Mar 3, 2022

View reviewed changes

Alexandre Pion added 4 commits March 3, 2022 19:44

Options to provide OpenSSL distinct inc/lib paths

b6a937e

Allow passing distinct OpenSSL paths for include and library files when configuring LDNS.

Pass custom OpenSSL paths to LDNS C compiler

97576ce

Add POD for optional features

a29ed65

Update DNS data used in tests

9e46c8d

The `se` and `nic.se` zones have evolved a little. The tests made over the network using these zones have been slightly updated to fix errors and be aligned with current zone configuration.

Editorial updates

9d23ca9

matsduf requested changes Mar 25, 2022

View reviewed changes

Alexandre Pion added 3 commits April 7, 2022 12:00

Fix default value in documentation

156cca2

Document new OpenSSL options in README

580888f

Update documentation

f39abb9

matsduf requested changes Apr 8, 2022

View reviewed changes

matsduf approved these changes Apr 27, 2022

View reviewed changes

ghost merged commit 337eacf into zonemaster:develop Apr 28, 2022

ghost mentioned this pull request May 23, 2022

Installation instructions for CentOS 7 zonemaster/zonemaster-engine#1080

Merged

ghost mentioned this pull request Aug 24, 2022

Options to build with Libidn and LDNS in uncommon locations #152

Merged

ghost deleted the build-configuration branch September 5, 2022 12:06

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Give more freedom when configuring Zonemaster::LDNS #134

Give more freedom when configuring Zonemaster::LDNS #134

ghost commented Mar 1, 2022 •

edited by ghost

Loading

mattias-p left a comment

mattias-p Mar 1, 2022

ghost commented Mar 3, 2022

matsduf left a comment

matsduf Mar 25, 2022

ghost Apr 7, 2022

matsduf Mar 25, 2022

ghost Apr 7, 2022

ghost commented Apr 7, 2022

matsduf left a comment

matsduf Apr 8, 2022

ghost Apr 11, 2022

matsduf Apr 11, 2022

ghost Apr 11, 2022

matsduf Apr 11, 2022

ghost Apr 11, 2022

matsduf Apr 11, 2022

ghost Apr 12, 2022

matsduf Apr 13, 2022

ghost commented Apr 11, 2022 •

edited by ghost

Loading

matsduf commented Apr 11, 2022

		print "WARNING 'openssl-inc' option overridden by 'prefix-openssl'\n" if ( $opt_openssl_inc );
		print "WARNING 'openssl-lib' option overridden by 'prefix-openssl'\n" if ( $opt_openssl_lib );

		Enable (or disable) support for converting domain names from unicode to A-label
		format.

Give more freedom when configuring Zonemaster::LDNS #134

Give more freedom when configuring Zonemaster::LDNS #134

Conversation

ghost commented Mar 1, 2022 • edited by ghost Loading

Purpose

Context

Changes

How to test this PR

Note

mattias-p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Mar 3, 2022

matsduf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 7, 2022

matsduf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 11, 2022 • edited by ghost Loading

matsduf commented Apr 11, 2022

ghost commented Mar 1, 2022 •

edited by ghost

Loading

ghost commented Apr 11, 2022 •

edited by ghost

Loading