Fix build in place-with-space #145
Conversation
mohawk2
force-pushed
the
fix-in-space
branch
2 times, most recently
from
8d830ec to
576d035
Compare
|
Text::ParseWords::shellwords is a great idea |
|
@dk Thanks. Please could you merge and release this? |
|
I'd rather wait for you to answer my questions in the review first.. |
The "review" UI is pretty clear about reviews being pending. This means that, as here, you haven't actually sent it yet. Until you do, it will be very significantly difficult for me to answer any questions you may have. |
| @@ -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]); | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -2868,14 +2867,14 @@ WriteMakefile( | |||
| PREREQ_PM => \%PREREQ, | |||
| OBJECT => "@o_files", | |||
| INC => | |||
| join(' ', map { "-I$_" } @INCPATH ). | |||
| join(' ', map qq{"-I$_"}, @INCPATH ). | |||
There was a problem hiding this comment.
I get the idea, but why not maybe_quote?
There was a problem hiding this comment.
Because that is how EUMM does it (unconditionally), which works great in all environments.
There was a problem hiding this comment.
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
|
I don't understand why do you want a formal review instead of just reading comments under the proposed commits, but if it helps, well... here it goes |
Until today the only comment visible on here was "Text::ParseWords::shellwords is a great idea". You said above you'd "rather wait for [me] to answer my questions in the review first" about the changes I made here. No such questions were visible to me or anyone else, at all. Either the "questions" were a GitHub review you'd written but not published (my assumption), or one of us doesn't know how to ask questions. |
|
That's not true that the only comment was visible is the one above; my comments on the commits were visible for 23 days. I also get mails for your answers under them today, so it looks like these were published alright. Now, I added some text under both, hopefully you can see them. In short though, the first commit with 0,1 I don't find necessary, while the 2nd with the qq is okay but I would prefer maybe_quote. If you would address these I shall merge the pr |
Thanks for the new version! With these changes (plus these fixes for PkgConfig: PerlPkgConfig/perl-PkgConfig#61), Prima now builds cleanly on my "place-with-space" Windows setup.