Discussion:
gnulib-tool.py: follow gnulib-tool changes
(too old to reply)
Bruno Haible
2017-09-09 09:44:14 UTC
Permalink
Raw Message
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
Raw Message
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.

2017-09-09 12:44 GMT+03:00 Bruno Haible <***@clisp.org>:

> 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
Raw Message
On Samstag, 9. September 2017 13:08:09 CEST Dmitry Selyutin wrote:
> 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
Raw Message
Hello Tim,

> 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.

> @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
Raw Message
On 09/09/2017 09:38 AM, Bruno Haible wrote:
> Hello Tim,
>
>> 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.
>
>> @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
Raw Message
On Samstag, 9. September 2017 09:51:01 CEST Eric Blake wrote:
> On 09/09/2017 09:38 AM, Bruno Haible wrote:
> > Hello Tim,
> >
> >> 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.
> >
> >> @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
Raw Message
Hi Dmitry,

Thanks for the reviews.

> 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,

> 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
Raw Message
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
Raw Message
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.

2017-09-09 17:52 GMT+03:00 Bruno Haible <***@clisp.org>:

> 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
Raw Message
Hi Dmitry,

> > [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.

> 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?

> > [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
Raw Message
> 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.

> 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.

2017-09-09 18:41 GMT+03:00 Bruno Haible <***@clisp.org>:

> Hi Dmitry,
>
> > > [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.
>
> > 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?
>
> > > [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...