Skip to content

Fix build in place-with-space #145

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix build in place-with-space #145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 11 additions & 12 deletions Makefile.PL
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use File::Find;
use File::Path;
use File::Basename;
use File::Copy;
use Text::ParseWords qw(shellwords);

use vars qw(
$ARGV_STR
Expand Down Expand Up @@ -1363,17 +1364,15 @@ sub pkgconfig
$MODVERSIONS{$package} = $modversion;
printlog "$modversion\n";

my %h;

%h = map { $_ => 1 } @INCPATH;
my %h = map { $_ => 1 } @INCPATH;
my $inc = `$PKGCONFIG --cflags-only-I $package`;
if ( $? != 0 ) { # package has error?
printlog "misconfigured\n";
return 0;
}
chomp $inc;
$inc =~ s/\-I//g;
for ( split " ", $inc ) {
for ( shellwords $inc ) {
push @INCPATH, $_ unless $h{$_};
}

Expand All @@ -1385,7 +1384,7 @@ sub pkgconfig
}
chomp $libs;
$libs =~ s/\-l//g;
for ( split " ", $libs ) {
for ( shellwords $libs ) {
push @LIBS, $_ unless $h{$_};
}

Expand All @@ -1397,7 +1396,7 @@ sub pkgconfig
}
chomp $libpath;
$libpath =~ s/\-[LR]//g;
for ( split " ", $libpath ) {
for ( shellwords $libpath ) {
push @LIBPATH, $_ unless $h{$_};
}

Expand Down Expand Up @@ -2263,7 +2262,7 @@ sub create_codecs_c
print $fh <<CONTENT;
/*
This file was automatically generated.
Do not edit, you'll loose your changes anyway.
Do not edit, you'll lose your changes anyway.
*/

#include "img.h"
Expand Down Expand Up @@ -2341,7 +2340,7 @@ sub create_config_pm
$ip[0] = '$(lib)' . "/Prima/CORE";
$ip[1] = '$(lib)' . "/Prima/CORE/generic";
my $ippi = join(',', map {"\'$_\'"} @ip);
my $inci = join(' ', map maybe_quote("-I$_"), @ip);
my $inci = join(' ', (map qq{"-I$_"}, @ip[0,1]), map maybe_quote("-I$_"), @ip[2..$#ip]);
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand this. How is it 0,1 is special, and why forcing double quotes on unix? Also how come maybe_quote doesn't work for your case, if it very specifically addresses spaces? Looks very wrong to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you look 2 lines up, you will see that the first two entries in @ip can never have spaces in them, since they will look like $(lib)/Prima/CORE. Until, that is, make expands the $(lib) into something that might have a space in. That is much later, far after maybe_quote can be involved, and this construct protects from that.

Copy link
Owner

Choose a reason for hiding this comment

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

Okay I see the logic behind but I would prefer to keep this as is because 1) it's shorter 2) special cases of 0,1 may change in future and there is a slight chance that the corresponding slice will be forgotten 3) basically, because it is a special case for no reason.


# libs
my @libpath = @LIBPATH;
Expand Down Expand Up @@ -2371,7 +2370,7 @@ sub create_config_pm
open my $fh, ">", "Prima/Config.pm" or die "cannot open Prima/Config.pm:$!\n";
print $fh <<CONFIG;
# This file was automatically generated.
# Do not edit, you'll loose your changes anyway.
# Do not edit, you'll lose your changes anyway.
package Prima::Config;
use strict;
use warnings;
Expand Down Expand Up @@ -2868,14 +2867,14 @@ WriteMakefile(
PREREQ_PM => \%PREREQ,
OBJECT => "@o_files",
INC =>
join(' ', map { "-I$_" } @INCPATH ).
join(' ', map qq{"-I$_"}, @INCPATH ).
Copy link
Owner

Choose a reason for hiding this comment

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

I get the idea, but why not maybe_quote?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because that is how EUMM does it (unconditionally), which works great in all environments.

Copy link
Owner

Choose a reason for hiding this comment

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

Fair enough, but I think that conditionally quoted lines look better than unconditionally quoted. I rarely copypaste the individual compilation or linking lines and run them as standalone, and they are mostly ugly enough as they are, without quotes. The idea is good but let's use maybe_quote still, at least it is systematically used throughout makefile.pl

' ' . $cmd_options{EXTRA_CCFLAGS},
LIBS => [
$cmd_options{EXTRA_LDFLAGS} . ' ' .
$LDFLAGS . ' ' .
':nosearch ' .
join(' ', map { "-L$_" } @LIBPATH) . ' ' .
join(' ', map { "-l$_" } @LIBS),
join(' ', map qq{"-L$_"}, @LIBPATH) . ' ' .
join(' ', map "-l$_", @LIBS),
],
LICENSE => 'FREEBSD',
EXE_FILES => \@exe_files,
Expand Down
2 changes: 1 addition & 1 deletion utils/prima-tmlink.pl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ sub load_file

print $f <<HEAD;
/* This file was automatically generated.
Do not edit, you'll loose your changes anyway.
Do not edit, you'll lose your changes anyway.
file: $fName */
HEAD

Expand Down
Loading