Discussion:
gnulib-tool.py: follow gnulib-tool changes
Bruno Haible
2017-09-09 09:44:14 UTC
Permalink
Hi Dmitry,

I've started to copy all modifications done to gnulib-tool since 2012
over to gnulib-tool.py. Here is the first round of such updates.
OK to push?

Most of these are not really tested, and I'm a beginner regarding Python.
Therefore it's very possible that you find mistakes in these patches.

Bruno
Dmitry Selyutin
2017-09-09 10:08:09 UTC
Permalink
Hi Bruno,

I have only one question regarding --no-changelog option.
Are you sure you want to entirely deprecate it?
Because right now it is a no-op, but parser still doesn't bark when it
founds it.
The current behavior of gnulib-tool.py matches the same of gnulib-tool.
Once you will apply the change, parser shall complain on unknown
--no-changelog,
And this change will for sure break some setups (e.g.wget2 still uses it).
CC'ing Tim.

As for the rest, feel free to push the changes.
I'll do the same in the "python" branch.
Post by Bruno Haible
Hi Dmitry,
I've started to copy all modifications done to gnulib-tool since 2012
over to gnulib-tool.py. Here is the first round of such updates.
OK to push?
Most of these are not really tested, and I'm a beginner regarding Python.
Therefore it's very possible that you find mistakes in these patches.
Bruno
--
With best regards,
Dmitry Selyutin
Tim Rühsen
2017-09-09 12:54:33 UTC
Permalink
Post by Dmitry Selyutin
Hi Bruno,
I have only one question regarding --no-changelog option.
Are you sure you want to entirely deprecate it?
Because right now it is a no-op, but parser still doesn't bark when it
founds it.
The current behavior of gnulib-tool.py matches the same of gnulib-tool.
Once you will apply the change, parser shall complain on unknown
--no-changelog,
And this change will for sure break some setups (e.g.wget2 still uses it).
CC'ing Tim.
We don't use it explicitly. We use the 'bootstrap' script from gnulib which
includes --no-changelog:

#######
# Import from gnulib.

gnulib_tool_options="\
--import\
--no-changelog\
--aux-dir $build_aux\
--doc-base $doc_base\
--lib $gnulib_name\
--m4-base $m4_base/\
--source-base $source_base/\
--tests-base $tests_base\
--local-dir $local_gl_dir\
$gnulib_tool_option_extras\
"
#######

So if you remove it from bootstrap, I'll update bootstrap in our project main
dir and that's it.

@Bruno Maybe it would be wise to just have a symlink to gnulib/build-aux/
bootstrap ?

Regards, Tim
Bruno Haible
2017-09-09 14:38:57 UTC
Permalink
Hello Tim,
Post by Tim Rühsen
So if you remove it from bootstrap, I'll update bootstrap in our project main
dir and that's it.
No, we don't remove it from 'bootstrap'. This piece of backward-compatibility
code is not expensive to keep.
Post by Tim Rühsen
@Bruno Maybe it would be wise to just have a symlink to gnulib/build-aux/
bootstrap ?
I don't understand what you mean. Symlinks inside git repositories are a
problem for those people who perform a checkout on Windows.

Bruno
Eric Blake
2017-09-09 14:51:01 UTC
Permalink
Post by Bruno Haible
Hello Tim,
Post by Tim Rühsen
So if you remove it from bootstrap, I'll update bootstrap in our project main
dir and that's it.
No, we don't remove it from 'bootstrap'. This piece of backward-compatibility
code is not expensive to keep.
Post by Tim Rühsen
@Bruno Maybe it would be wise to just have a symlink to gnulib/build-aux/
bootstrap ?
I don't understand what you mean. Symlinks inside git repositories are a
problem for those people who perform a checkout on Windows.
Another problem is that bootstrap MUST be a shell script rather than a
symlink, since one of the purposes of running ./bootstrap is to DO the
git submodule checkout. If it is checked into git as a symlink to a
submodule, then users have to run something other than ./bootstrap to
get the submodule updated, which defeats the purpose of bootstrap being
able to do that itself. Yes, that makes bootstrap special as one of the
few files that must be copied directly into projects using it, rather
than left as a symlink into a submodule.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Tim Rühsen
2017-09-09 19:15:37 UTC
Permalink
Post by Eric Blake
Post by Bruno Haible
Hello Tim,
Post by Tim Rühsen
So if you remove it from bootstrap, I'll update bootstrap in our project
main dir and that's it.
No, we don't remove it from 'bootstrap'. This piece of
backward-compatibility code is not expensive to keep.
Post by Tim Rühsen
@Bruno Maybe it would be wise to just have a symlink to gnulib/build-aux/
bootstrap ?
I don't understand what you mean. Symlinks inside git repositories are a
problem for those people who perform a checkout on Windows.
Another problem is that bootstrap MUST be a shell script rather than a
symlink, since one of the purposes of running ./bootstrap is to DO the
git submodule checkout.
Lol, yes that's pretty obvious - shame on me (guess I was too tired ;-)
My question was at least so stupid that Bruno couldn't understand it.

Regards, Tim

Bruno Haible
2017-09-09 14:35:21 UTC
Permalink
Hi Dmitry,

Thanks for the reviews.
Post by Dmitry Selyutin
Because right now it is a no-op, but parser still doesn't bark when it
founds it.
The current behavior of gnulib-tool.py matches the same of gnulib-tool.
Once you will apply the change, parser shall complain on unknown
--no-changelog,
Good point. I missed this.

Pushed with this parser change withdrawn,
Post by Dmitry Selyutin
As for the rest, feel free to push the changes.
I'll do the same in the "python" branch.
Yes, presumably you will have do adapt these changes from the 'master' to
the 'python' branch. I'm only doing the translation from past gnulib-tool
history to gnulib-tool.py in 'master'.

Bruno
Bruno Haible
2017-09-09 14:52:37 UTC
Permalink
Hi Dmitry,

Here's the next round of patches, for your review. I'm slowly working through
the past gnulib-tool changes.

Bruno
Dmitry Selyutin
2017-09-09 15:21:28 UTC
Permalink
Hi Bruno,


thanks for these patches. I've left some comments below.
[PATCH 6/6] gnulib-tool.py: follow gnulib-tool changes, part 14
gnulib-tool: don't transform binary files with sed
All these sed transformers shall be IMHO entirely deprecated. I don't quite
remember why I used sed; may be I could not find a way to express the same
idea
using Python "re" module.
Anyway, feel free to push it, because right now it will make the behavior
more
stable; be aware though that this part of code is going to be removed.
[PATCH 5/6] gnulib-tool.py: follow gnulib-tool changes, part 13
gnulib-tool: concatenate lib_SOURCES to a single line
A bit tricky one, but OK from my side. The only thing I noted is that
`startpos,pos = match.span()` can be a bit better formatted into
`(startpos, pos) = match.span()`.



Everything is OK though, feel free to push.
Hi Dmitry,
Here's the next round of patches, for your review. I'm slowly working through
the past gnulib-tool changes.
Bruno
--
With best regards,
Dmitry Selyutin
Bruno Haible
2017-09-09 15:41:36 UTC
Permalink
Hi Dmitry,
Post by Dmitry Selyutin
[PATCH 6/6] gnulib-tool.py: follow gnulib-tool changes, part 14
gnulib-tool: don't transform binary files with sed
All these sed transformers shall be IMHO entirely deprecated. I don't quite
remember why I used sed
Using 'sed' is acceptable here because the input comes from a file and the
output goes to a file anyway. If you replace 'sed' here, you save a subprocess
invocation, though. This will be interesting when you/we are going to start
optimizing the thing.

Another possible optimization here is that first, we do a
cp lookedup tmpfile
and then
sed -e transformer < lookedup > tmpfile
We could eliminate the cp command when there is a transformer.
Post by Dmitry Selyutin
be aware though that this part of code is going to be removed.
Well, the logic that binary files (*.mo, *.class) should be copied as-is,
not transformed, should be kept, no? You'll replace the implementation
of the transform?
Post by Dmitry Selyutin
[PATCH 5/6] gnulib-tool.py: follow gnulib-tool changes, part 13
gnulib-tool: concatenate lib_SOURCES to a single line
A bit tricky one, but OK from my side. The only thing I noted is that
`startpos,pos = match.span()` can be a bit better formatted into
`(startpos, pos) = match.span()`.
Done. I had verified that both syntaxes work, but did not know which one is
the preferred one.

Bruno
Dmitry Selyutin
2017-09-09 16:04:13 UTC
Permalink
Post by Bruno Haible
Well, the logic that binary files (*.mo, *.class) should be copied as-is,
not transformed, should be kept, no? You'll replace the implementation
of the transform?
The binary files shall not be touched, but text files shall not be processed
with sed. That's what I mean; the patch is OK, but the code around is
flawed.
Post by Bruno Haible
If you replace 'sed' here, you save a subprocess
invocation, though.
Exactly; I think one of the strongest motivations to perform the whole
rewrite
in Python is the fact that the original gnulib-tool spawns processes too
often.
Moreover, process invocation is quite a dumb technique when you have
built-in
language features instead.
Post by Bruno Haible
Hi Dmitry,
Post by Dmitry Selyutin
[PATCH 6/6] gnulib-tool.py: follow gnulib-tool changes, part 14
gnulib-tool: don't transform binary files with sed
All these sed transformers shall be IMHO entirely deprecated. I don't
quite
Post by Dmitry Selyutin
remember why I used sed
Using 'sed' is acceptable here because the input comes from a file and the
output goes to a file anyway. If you replace 'sed' here, you save a subprocess
invocation, though. This will be interesting when you/we are going to start
optimizing the thing.
Another possible optimization here is that first, we do a
cp lookedup tmpfile
and then
sed -e transformer < lookedup > tmpfile
We could eliminate the cp command when there is a transformer.
Post by Dmitry Selyutin
be aware though that this part of code is going to be removed.
Well, the logic that binary files (*.mo, *.class) should be copied as-is,
not transformed, should be kept, no? You'll replace the implementation
of the transform?
Post by Dmitry Selyutin
[PATCH 5/6] gnulib-tool.py: follow gnulib-tool changes, part 13
gnulib-tool: concatenate lib_SOURCES to a single line
A bit tricky one, but OK from my side. The only thing I noted is that
`startpos,pos = match.span()` can be a bit better formatted into
`(startpos, pos) = match.span()`.
Done. I had verified that both syntaxes work, but did not know which one is
the preferred one.
Bruno
--
With best regards,
Dmitry Selyutin
Loading...