Re: [Freeipa-devel] [PATCH] 0066 Arrange stripping .po files

[Petr Viktorin]

On 07/19/2012 10:52 PM, John Dennis wrote:

On 06/25/2012 07:17 AM, Petr Viktorin wrote:

The translation files we currently store in Git are full of redundant
information: source strings for untranslated messages, and file
locations.
The first causes unnecessarily huge files. The second makes diffs
unreadable: when code is edited and line numbers change, metadata for
all messages shows up as changed. This makes reviewing translation
patches, and merging possible conflicts, hard -- it requires specialized
tools.

This patch changes the Makefile to strip the unneeded data from .po
files.

Translators using Git must now run msgmerge (or, `make merge-po`) to get
.po files they can work with. Transifex users are unaffected, as the
source .pot file is not changed.

The i18n tests use file locations for producing nice error reports¹.
To make this work as before, the .pot is merged in before validation to
restore comments.
Currently this takes a noticeable amount of time, because polib uses a
particularly naïve algorithm for merging. I've sent a patch to polib to
resolve this; once that makes it downstream merging will be fast again.

Updating the translations with the new Makefile will cause a 5MB patch.
I don't want to pollute the mailing list with it, at least until the
Makefile patch is reviewed. It's available
https://github.com/encukou/freeipa/commit/65e2e4.patch


https://fedorahosted.org/freeipa/ticket/2435


--
¹ And for divining the programming language messages come from, but that
is only done on the .pot file, unaffected by this patch.


Good work and it's very close to getting an ACK.

There is now a discrepancy between what the Makefile thinks is the list
of po files and the actual list of po files after running strip-po. This
causes confusing errors.

I think the source of this problem is the Makefile has a list of po
files in the variable $(po_files)

For starters why is:

strip-po:
 @for po_file in $$(ls *.po); do \

instead of:

strip-po:
 @for po_file in $(po_files); do \


Good catch, I'll update it to be consistent with the status quo.
But see below.


If you run make validate-po before running make strip-po you get:

5 errors in 21 files

After stripping the po files make validate-po gives you:

14 errors in 21 files


I left updating the files to a subsequent patch 
(https://github.com/encukou/freeipa/commit/65e2e4.patch); the LINGUAS 
update is part of that.



The extra 9 errors are due to the fact validate-po is being asked to
validate a non-existent po file which it considers an error (which I
believe is a correct check).

make msg-stats gets confused for the same reason, it's asked to
examine files that no longer exist.

make mo-files now fails catastrophically for the same reason, it's
being asked to operate on files that don't exist.

In general large parts of the Makefile will now be confused or generate
errors because the file list is incorrect.


Somehow we have to align the list of po files. That presents all sorts
of interesting questions:

* does the list come from the LINQUAS file? (current method)

* does the list come from git? Doesn't work if you're not in a git
development tree. This problem is easily seen when the RPM's are built.
No file list can be generated because there is no git repo so you end up
with 0 files being passed to the validation commands. Since validation
is not critical when building RPM's this hasn't been a show stopper but
it really needs to be fixed in some way at some point.


I agree that tying ourselves to Git isn't a nice thing to do. I know I 
am never happy when I can't compile some project in Mercurial after 
importing it to Git :)


If we use the ls-files strategy then that should at least write the list 
to a version-controlled file, which we fall back to in case we're not in 
a git tree.



* does the list come from the current directory contents? What you did
with strip-po, but that also has a potential for errors. What if someone
deletes or adds a file in their tree by mistake?


I personally would do this -- the most straightforward way to do it. If 
someone adds or deletes a file by mistake, a `git status` will reveal 
it. We could have a sanity check that refuses to build if there is a 
discrepancy between Git and the working tree (of course outside of a Git 
repo it would just warn).


There's one more reason for going with directory contents: when you're 
pulling from Transifex or otherwise adding/removing the translation 
files, you have to carefully keep LINGUAS in sync with the tree, 
otherwise the tools can either blow up or do too little. Debugging that 
could be frustrating. Having the tools look in the directory itself, and 
only doing sanity checking at a point where everything should be in 
order, should make everything easier.



* should make strip-po edit the LINGUAS file? (maybe the best
solution). Maybe when it detects an empty file and removes it it should
run a sed command to delete the line in LINGUAS?

Re: [Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater

[Petr Viktorin]

On 07/19/2012 08:01 PM, John Dennis wrote:

Overall I really like the approach, good work.

I have not applied and run the patch, so my comments are based only on
code reading.

I have just a small things which might need changing.

One of the ideas in the log manager is to easily support per class
logging (borrowed from Java). This allows you to enable/disable or
otherwise configure logging per logical code unit as opposed to a big
global hammer (i.e. root logger). It also allows for the class name
(logger name) to show up in the log output which can be really handy so
you know which class is emitting a message (we don't currently turn this
on but it's a trival one-line change to the log format spec.).

Each significant class should initialize it's own logging, which is very
easy to do with this line at the top of a class's __init__()

 log_mgr.get_logger(self, True)

so instead of:

 self.logger.info()

you would say:

 self.info()

A fair amount of the code in the framework is doing this now, but the
install code was never cleaned up. That was left for another day, I
guess that day is here.


Updated. I also added the necessary lint exception.
I'm curious as to why it works that way, though. Normally, to put 
methods in a class, you would use a base class/mixin. Why this dynamic 
magic?



Also each log method (i.e. info(), warn(), error(), etc) should pass
it's format substitutions as parameters rather than preformatting the
message before submitting it to the logger. This is a performance
optimization because the logger delays building the message until it
knows the message will actually be emitted and not discarded (common
case with debug messages).

So instead of:

 self.debug(the contents of this big dict is: %s % my_dict)

You would write:

 self.debug(the contents of this big dict is: %s, my_dict)

A small different in coding, but a lot less work at run time.


Fixed, thanks for noticing.


I'm not sure the following message handling is optimal

+message = '\n'.join(
+The hostname resolves to the localhost address
(127.0.0.1/::1),
+Please change your /etc/hosts file so that the hostname,
+resolves to the ip address of your network interface.,
+,
+Please fix your /etc/hosts file and restart the setup
program,


I'm not a big fan of individual lines of text in the source that are
glued together or are emitted individually via a series of print
statements. I'd rather see multi-line text as multi-line text using
Python's multi-line string operator (e.g. ''' or ). That way you can
see the text as it's meant to appear, but there is a bigger reason.


You're right, fixed. To keep sane indentation I used textwrap.dedent.


Shouldn't this message be internationalized? (e.g. wrapped in _()). When
a message is internationalized it's even more important for multi-line
text to appear in it entirety to give translators the greatest context
and latitude for their translation rather than arbitrarily splitting it
up into fragments according to English norms (fragments that might not
even translate in another language).

Also, a quick glance suggests a number of the messages need i18n treatment.


None of the tools are internationalized. Perhaps they need to be, but it 
should be a separate ticket.



Overall though, I really love the approach, great work!


Thanks for the review :)

--
Petr³


From cfa0d80b78bf9ea8651a0364703154c47eae9ff6 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Fri, 20 Apr 2012 04:39:59 -0400
Subject: [PATCH] Framework for admin/install tools, with ipa-ldap-updater

Currently, FreeIPA's install/admin scripts are long pieces of code
that aren't very reusable, importable, or testable.
They have been extended over time with features such as logging and
error handling, but since each tool was extended individually, there
is much inconsistency and code duplication.
This patch starts a framework which the admin tools can use, and
converts ipa-ldap-updater to use the framework.

Common tasks the tools do -- option parsing, validation, logging
setup, error handling -- are represented as methods. Individual
tools can extend, override or reuse the defaults as they see fit.

The ipa-ldap-updater has two modes (normal and --upgrade) that
don't share much functionality. They are represented by separate
classes. Option parsing, and selecting which class to run, happens
before they're instantiated.

All code is moved to importable modules to aid future testing. The
only thing that remains in the ipa-ldap-updater script is a two-line
call to the library.

First part of the work for:
https://fedorahosted.org/freeipa/ticket/2652
---
 install/tools/ipa-ldap-updater|  206 -
 ipapython/admintool.py|  229 +
 ipaserver/install/installutils.py |   92 ++---
 ipaserver/install/ipa_ldap_updater.py |  189 

[Freeipa-devel] [PATCH 0038] Fix two memory leaks in ldap_query()

[Petr Spacek]

Hello,

this patch fixes two memory leaks in ldap_query(). Both memory leaks occurs 
after non-success queries.


It effectively re-implements fix for ldap_query can incorrectly return 
ISC_R_SUCCESS even when failed:

http://git.fedorahosted.org/git/?p=bind-dyndb-ldap.git;a=commitdiff;h=a7cd8ae747b3a81a02ab9e5dbefe1c595aa24ff6

Please double-check this approach.

Thanks.

Petr^2 Spacek
From c8718b98641e7537b2350a625b03b0b7fec6f206 Mon Sep 17 00:00:00 2001
From: Petr Spacek pspa...@redhat.com
Date: Fri, 20 Jul 2012 14:18:41 +0200
Subject: [PATCH] Fix two memory leaks in ldap_query().

Signed-off-by: Petr Spacek pspa...@redhat.com
---
 src/ldap_helper.c |   14 --
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index 6ac76faecbc250deb28203256fa13d83ae6c80f4..daffac7c7825a99a07c333217638d3beaddfaad2 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -1753,28 +1753,30 @@ retry:
 	   ldap_qresult-ldap_entries);
 		if (result != ISC_R_SUCCESS) {
 			log_error(failed to save LDAP query results);
-			return result;
+			goto cleanup;
 		}
 
 		*ldap_qresultp = ldap_qresult;
 		return ISC_R_SUCCESS;
+	} else {
+		result = ISC_R_FAILURE;
 	}
 
 	ret = ldap_get_option(ldap_conn-handle, LDAP_OPT_RESULT_CODE,
 			  (void *)ldap_err_code);
-	if (ret == LDAP_OPT_SUCCESS  ldap_err_code == LDAP_NO_SUCH_OBJECT)
-		return ISC_R_NOTFOUND;
-	/* some error happened during ldap_search, try to recover */
-	else if (!once) {
+	if (ret == LDAP_OPT_SUCCESS  ldap_err_code == LDAP_NO_SUCH_OBJECT) {
+		result = ISC_R_NOTFOUND;
+	} else if (!once) {
+		/* some error happened during ldap_search, try to recover */
 		once++;
 		result = handle_connection_error(ldap_inst, ldap_conn,
 		 ISC_FALSE);
 		if (result == ISC_R_SUCCESS)
 			goto retry;
 	}
 cleanup:
 	ldap_query_free(ISC_FALSE, ldap_qresult);
-	return ISC_R_FAILURE;
+	return result;
 }
 
 /**
-- 
1.7.7.6

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 0066 Arrange stripping .po files

[John Dennis]

Great I agree with everything you said.

I'm happy to have the file list be derived from the directory contents. 
Are you planning on doing that in another patch?


FWIW the LINGUAS file was a holdover from when we first set this up 
based exclusively on GNU gettext suggested examples. As things have 
evolved it no longer makes sense. Also the contributing translators file 
is now out of date and was from an earlier era when translators emailed 
.po files to us, so it was easy to maintain. Now that everything is TX 
based we should probably nuke that file or figure out someway to extract 
the contributors from either TX or the po files. I'm not sure we're even 
giving credit to the translators anymore, but we should.


But... I do have one final issue/question. I missed this in the first 
review. po_files is now dependent on update-pot instead of the pot file. 
We had decided that we were only going to regenerate the pot file on 
demand at specific times. Won't this dependency change cause the pot 
file to be updated frequently? (I realize only in the local tree). Note 
that when we run the validations we generate a temporary pot file from 
the current contents of the tree specifically to avoid overwriting the 
pot file.


I suppose having a conversation about when the pot file gets updated is 
a good one to have, we don't do it often enough IMHO. But I'm not sure 
it's correct to modify a file under SCM control if it wasn't intentional.


--
John Dennis jden...@redhat.com

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/


___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater

[John Dennis]

A fair amount of the code in the framework is doing this now, but the
install code was never cleaned up. That was left for another day, I
guess that day is here.


Updated. I also added the necessary lint exception.
I'm curious as to why it works that way, though. Normally, to put
methods in a class, you would use a base class/mixin. Why this dynamic
magic?


Good question. I don't like dynamic magic either, it makes static code 
reading difficult and diminishes the value of static code 
analysis/checking. Jason who originally wrote the framework used dynamic 
magic a fair amount, including the initialization of the logging methods 
in the plugins. When I wrote the log manager I kept Jason's existing 
strategy (how many things do change at once?). In hindsight a mixin 
would have been a better solution, something we should probably fix down 
the road.


Everything looks good, although there might be one minor thing that 
needs fixing. Shouldn't the logging setup be done first? That way any 
code that executes has the ability to emit a message. For example what 
if validate_options() wants to emit a message?


--
John Dennis jden...@redhat.com

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/


___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater

[Petr Viktorin]

On 07/20/2012 05:59 PM, John Dennis wrote:

A fair amount of the code in the framework is doing this now, but the
install code was never cleaned up. That was left for another day, I
guess that day is here.


Updated. I also added the necessary lint exception.
I'm curious as to why it works that way, though. Normally, to put
methods in a class, you would use a base class/mixin. Why this dynamic
magic?


Good question. I don't like dynamic magic either, it makes static code
reading difficult and diminishes the value of static code
analysis/checking. Jason who originally wrote the framework used dynamic
magic a fair amount, including the initialization of the logging methods
in the plugins. When I wrote the log manager I kept Jason's existing
strategy (how many things do change at once?). In hindsight a mixin
would have been a better solution, something we should probably fix down
the road.


Thinking more about this, composition would probably be cleaner than 
inheritance here (i.e. using self.logger.warn(...) instead of mixing the 
functionality into the class itself).

Well, one day the time to fix it will come.


Everything looks good, although there might be one minor thing that
needs fixing. Shouldn't the logging setup be done first? That way any
code that executes has the ability to emit a message. For example what
if validate_options() wants to emit a message?



That's a good question.
If we set up logging before validating the options, we'll log all 
invocations, including those with misspelled option names and forgotten 
sudos. Do we want that?


--
Petr³

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 0066 Arrange stripping .po files

[John Dennis]

On 07/20/2012 12:28 PM, Petr Viktorin wrote:

On 07/20/2012 05:39 PM, John Dennis wrote:

Great I agree with everything you said.

I'm happy to have the file list be derived from the directory contents.
Are you planning on doing that in another patch?


Yes, I want to do it in a new patch.
It's a bit more complicated than it looks: creating a new translation
will work differently than just adding it to LINGUAS and running
create-po. The ticket is for beta 2 so I'd rather not start a new round
of reviews.


Fine with me to do that in another patch.

As for create-po, I think that's also holdover from pre-Transifex days. 
With Transifex I'd don't ever see a need to create an empty po file. Do 
you? Maybe we should just nuke the po creation in the Makefile.





FWIW the LINGUAS file was a holdover from when we first set this up
based exclusively on GNU gettext suggested examples. As things have
evolved it no longer makes sense. Also the contributing translators file
is now out of date and was from an earlier era when translators emailed
.po files to us, so it was easy to maintain. Now that everything is TX
based we should probably nuke that file or figure out someway to extract
the contributors from either TX or the po files. I'm not sure we're even
giving credit to the translators anymore, but we should.


Noted; when the discussion's done I'll file a ticket.


But... I do have one final issue/question. I missed this in the first
review. po_files is now dependent on update-pot instead of the pot file.
We had decided that we were only going to regenerate the pot file on
demand at specific times. Won't this dependency change cause the pot
file to be updated frequently? (I realize only in the local tree). Note
that when we run the validations we generate a temporary pot file from
the current contents of the tree specifically to avoid overwriting the
pot file.


Are the po files updated more often? I don't really see a reason to
merge the po files with an old pot.


What merge are you referring to? The only merge I'm aware of at the 
moment is during validation, but that merge is done from a temporary 
updated pot file that is current with the tree.


Any other merging is done by Transifex at the time we pull a po file.

The frequency of po update doesn't seem relevant, what is your concern 
in this regard?





I suppose having a conversation about when the pot file gets updated is
a good one to have, we don't do it often enough IMHO. But I'm not sure
it's correct to modify a file under SCM control if it wasn't intentional.


How is Transifex set up here? If it automatically picks up changes when
the pot file is modified, then we should back up the translations before
changing the pot, so we can't do it automatically.
Another wart is the line number cruft in the pot file -- any time it's
updated we'll get a huge diff, so it makes sense to update sparingly.


Transifex gives you two options for your pot file, either you tell TX 
the location of your pot file in a public SCM and it watches for updates 
and automatically pulls it when it changes in SCM -or- you manually push 
the pot file to TX.


We've been using the watch the pot file in git option. Thus whenever 
we commit a new version of the pot file all developers and TX get it 
simultaneously (well sort of).


If we do the manual push method the maintainer has to *both* commit to 
git *and* push to TX, so the former seems less error prone and more 
automated.


The idea was we would have a string freeze prior to release and/or 
periodic intervals during branch development to update the pot. But we 
haven't been good about hitting these. However, note a manual push 
suffers from the same somebody has to do it at the right moment problem.




If Transifex is not wired to the pot, we could even go as far as
removing it from SCM entirely -- it's entirely generated, and rebuilding
it takes less than a second.
We'd just have to update Transifex manually.


It currently is wired to the pot. You make a valid point about currently 
not needing to maintain the pot in SCM. When we first set up 
translations we weren't using TX so having the pot file in SCM was a 
necessity.


Personally I don't trust TX's data storage and I think there is value in 
having each pot we push to TX be recoverable from our SCM. When things 
blow up (and they do) it's really nice to be able to reassemble the 
pieces or at lease follow the trail of how things changed. In the past 
I've had to answer questions like How the heck did this string get into 
this po file? Such questions can only be answered if we have the pot 
file we gave the translator. TX doesn't maintain it so we have to (or at 
least I think there is value in it).


Perhaps you can read between the lines and detect I don't view TX as the 
epitome of stability and robustness. It's still young and they are still 
adding features and changing how it works (kinda like IPA :-)



Oh, one thing I'll ask about: the Makefile is 

Re: [Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater

[John Dennis]

On 07/20/2012 12:34 PM, Petr Viktorin wrote:

On 07/20/2012 05:59 PM, John Dennis wrote:

A fair amount of the code in the framework is doing this now, but the
install code was never cleaned up. That was left for another day, I
guess that day is here.


Updated. I also added the necessary lint exception.
I'm curious as to why it works that way, though. Normally, to put
methods in a class, you would use a base class/mixin. Why this dynamic
magic?


Good question. I don't like dynamic magic either, it makes static code
reading difficult and diminishes the value of static code
analysis/checking. Jason who originally wrote the framework used dynamic
magic a fair amount, including the initialization of the logging methods
in the plugins. When I wrote the log manager I kept Jason's existing
strategy (how many things do change at once?). In hindsight a mixin
would have been a better solution, something we should probably fix down
the road.


Thinking more about this, composition would probably be cleaner than
inheritance here (i.e. using self.logger.warn(...) instead of mixing the
functionality into the class itself).
Well, one day the time to fix it will come.


Everything looks good, although there might be one minor thing that
needs fixing. Shouldn't the logging setup be done first? That way any
code that executes has the ability to emit a message. For example what
if validate_options() wants to emit a message?



That's a good question.
If we set up logging before validating the options, we'll log all
invocations, including those with misspelled option names and forgotten
sudos. Do we want that?



Sure, hard to say. Why don't we leave it the way it is now and down the
road if it shows up as an issue we'll tweak it then.

ACK.

--
John Dennis jden...@redhat.com

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/


___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel