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

Fix CMake build warnings #360

Merged
merged 2 commits into from
Dec 9, 2024
Merged

Fix CMake build warnings #360

merged 2 commits into from
Dec 9, 2024

Conversation

oschwald
Copy link
Member

@oschwald oschwald commented Dec 9, 2024

Closes #359.

}
}

sub _make_lib_man_links {
my $target = shift;

my $header = read_file("$Bin/../include/maxminddb.h");
my $header = read_file("$Bin/../include/maxminddb.h")
or die "Failed to read header file: $!";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks to me like read_file might already handle errors for us.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, at the very least autodie didn't cover them. Sorry about that.

$file
);
$file,
) or die "Failed to edit file: $!";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise for edit_file as with read_file.

for my $proto ( $header =~ /^ *extern.+?(MMDB_\w+)\(/gsm ) {
open my $fh, '>', "$target/man/man3/$proto.3";
open my $fh, '>', "$target/man/man3/$proto.3"
or die "Failed to open file: $!";
print {$fh} ".so man3/libmaxminddb.3\n";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think print could fail too, but it looks like autodie already didn't handle that!

@@ -41,7 +40,7 @@ sub _make_man {

my $input = "$Bin/../doc/$name.md";
my $man_dir = "$target/man/man$section";
mkpath($man_dir);
mkpath($man_dir) or die "Failed to create directory $man_dir: $!";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly mkpath handles errors already as well, although its docs seem more ambiguous to me!

This reduced the dependencies. See #359.
@horgh horgh merged commit fff8dae into main Dec 9, 2024
29 checks passed
@horgh horgh deleted the greg/fix-warnings branch December 9, 2024 20:09
@oschwald oschwald mentioned this pull request Dec 9, 2024
@gmou3
Copy link
Contributor

gmou3 commented Dec 10, 2024

Thanks for the quick response! However, now another issue has been unraveled for me:

Can't locate File/Slurp.pm in @INC (you may need to install the File::Slurp module) (@INC entries checked: /home/giorgos/perl5/lib/perl5 /usr/lib/perl5/5.40/site_perl /usr/share/perl5/site_perl /usr/lib/perl5/5.40/vendor_perl /usr/share/perl5/vendor_perl /usr/lib/perl5/5.40/core_perl /usr/share/perl5/core_perl) at libmaxminddb/dev-bin/make-man-pages.pl line 9.
BEGIN failed--compilation aborted at libmaxminddb/dev-bin/make-man-pages.pl line 9.

By the way, should the man pages always be generated? It took some time for me (after installing the File::Slurp module that is). Could a flag to choose whether to generate them be useful?

@oschwald
Copy link
Member Author

If you use the released source packages, e.g., libmaxminddb-1.11.0.tar.gz, you will not need to generate the man pages. We don't check in generated files into the repo.

@gmou3
Copy link
Contributor

gmou3 commented Dec 10, 2024

I am using the repo as a submodule and I build it using CMake. I would prefer it if people who built my project wouldn't get warnings (now there still is one due to the nonstandard File::Slurp perl module).

@oschwald
Copy link
Member Author

We don't have plans to commit generated files to the repo. I think we would accept a PR to disable man page generation.

I'll go ahead and make a small PR to remove the dependency on non-core Perl modules as that seems beneficial generally, but the man-page generation will still depend on lowdown or pandoc being installed.

@gmou3
Copy link
Contributor

gmou3 commented Dec 11, 2024

Thanks again, seems to be working as intended! Apparently (from code inspection), it checks and handles the absence of lowdown and pandoc, so all is fine now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

CMake build issues
3 participants