Front page | perl.perl6.internals |
Postings from August 2008
Re: My Parrot 0.7.0 patches (cygwin)
From: Reini Urban
August 24, 2008 09:38
Re: My Parrot 0.7.0 patches (cygwin)
Message ID: firstname.lastname@example.org
2008/8/24 Allison Randal <email@example.com>:
> Reini Urban wrote:
>> You want one patch only against HEAD? That's easy.
>> But I dislike the idea, as it violates the usage of single tickets.
> This is different than the usual case as it's a collection of dependent
> patches that can't be evaluated independently. Splitting them out is
> actually more work for the reviewer/tester.
> I've created a branch cygwin070patches for testing this collection. To do
> platform or language testing, please check out that branch. Reini, please
> submit further changes as diffs against that branch instead of updates to
> your previous patch files.
> Overall the work is sane. It'll need a few changes before merging in:
> In runtime/parrot/library/config.pir you add commented-out code, and a
> mention that certain logic has to be reversed "when installed versions
> should run faster than source builds". Those sorts of configuration changes
> should never involve commenting out and uncommenting bits of configuration
> files. Make it a compile-time or run-time configuration flag instead.
Just was just an internal TODO comment, and is actually fixed. I will
delete the superflous comment.
> In lib/Parrot/Configure/Data.pm, you changed some double C<<>> Pod tags to
> single C<> Pod tags. But, those code items contain "=>" separating the
> key/value arguments, and the '>' in the arrow will terminate the code tag.
> The double C<<>> tags avoid terminating on single '>' (which is why they
> were double C<<>> tags in the first place). So, I reverted that file before
> committing. (See the output from 'perldoc' or any Pod parser on your
> modified file.)
> In lib/Parrot/Configure/Compiler.pm, I agree that 'CONDITIONED_LINE' and
> 'INVERSE_CONDITIONED_LINE' aren't the clearest names, but '+' and '-' are
> far less clear. Change them to something meaningful like 'SHOW_LINE_IF' and
> 'HIDE_LINE_IF'. We can note the change in DEPRECATED.pod now, and remove
> 'CONDITIONED_LINE' and 'INVERSE_CONDITIONED_LINE' after a standard
> deprecation cycle (one release).
#+ and #- are well known common lisp idioms (the most well-known
thats why the reverse polish notation in the logic is used. It's just
so much easier
to parse and understand.
> Also in lib/Parrot/Configure/Compiler.pm, change the Polish notation of
> "(and a b (not c d))" to a saner "(a and b not (c and d))".
This is ALGOL syntax you suggest is really not "sane" :)
> Though really,
> since you're not implementing the advanced conditions, delete the comment
> defining the interface and the TODO comments about implementing it, just add
> a TODO RT ticket or keep it on your private TODO list.
I'll implement it for sure, but later. First I wanted to bring out the
this is just convenience sugar.
> In config/gen/makefiles/pge.in, config/gen/makefiles/tge.in, and
> config/gen/makefiles/root.in, pick a more meaningful variable name than
> 'SHRPENV' and a more meaningful condition name than 'cygchkdll'.
SHRPENV is directly from per5, and cygchkdll is they only way to check
now for cygwin
without the 57548-CONDITIONED_LINE_enh.patch. After that we can check against
cygwin, the $^O. I really need some #+(and cygwin win32): sections.
> Also in config/gen/makefiles/root.in we don't need a makefile target for
> regenerating the makefile. Delete it. And you added a chunk of commented out
> code again. Delete it.
That was the framework to autodetect configuration changes, to get rid of
make clean before perl Configure.pl.
I do need to regenerate the makefile, that's why I added it for every makefile.
I'll rather add the missing bits to the missings deps, than delete this logic.
The commented-out section works in 90%, but has still a flaw somewhere,
which could lead to cycles. I'll debug this when I have more time.
> Since you've touched the config file and core PIR file for every language,
> this branch will need extensive platform and language testing before it can
> be merged in.
I tested it extensively for the cygwin package. I believe I'm the only one who
tested it at all because there were so many errors and still are.
And I begin to wonder how others dare to release packages without working
installables and with self.build make recipes.
deb, gentoo, freebsd, rpm, the win32 build, and now my self-build work, but
at least as Makefiles so we will have a standard.
Just test-installable is a bit of work TODO. I had to test this by hand so far.
> The branch is failing one test that passes in trunk, should be a quick fix:
Thanks for reviewing!