Project

General

Profile

Feature #15355

Feature #10034: Translation web platform

Make the ikiwiki PO plugin able to update PO files for languages that are disabled on the website

Added by u over 1 year ago. Updated about 2 months ago.

Status:
In Progress
Priority:
Normal
Assignee:
Category:
Internationalization
Target version:
Start date:
03/01/2018
Due date:
% Done:

80%

Estimated time:
26.00 h
Feature Branch:
https://salsa.debian.org/tails-team/ikiwiki/tree/feature/15355-po-plugin-disable-languages
Type of work:
Communicate
Blueprint:
Starter:
Affected tool:
Translation Platform

Description

It might happen that we have po files of languages that we do not want to activate yet. In that case, ikiwiki will interpret these po files as mdwn and this breaks the wiki.
We might actually want to carry such translations after all, and not have them only in weblate. But we want ikiwiki on our production and local websites to ignore these language files while our staging website should render them.

Upstream todo/MR: https://ikiwiki.info/todo/_Add_ikiwiki_PO-Plugin_to_ignore_languages/

Another option is to add \.ru\.po$ (to disable Russian) to the exclude config variable in ikiwiki.setup.
But even if it works it's a bit lower-level and much harder to query/reconfigure programmatically. which I assume we may want to do both in tails.git and on the production ikiwiki.setup if the list of languages for which we carry PO files in tails.git changes often or without much coordination.
If the po_disabled_languages thing is easy it's much better. better semantics. Can be used elsewhere in the PO plugin if needed.

Also e.g. the staging website build script could simply empty po_disabled_languages, that can be done on the ikiwiki cmdline contrary to tweaking "excludes".

0001-Fix-user-visible-strings-and-messages-in-documentati.patch View (3.16 KB) u, 11/02/2018 06:14 PM

log.txt View (23.2 KB) lamby, 01/06/2019 05:27 PM


Related issues

Related to Tails - Feature #15081: Adjust our production website Rejected 12/19/2017
Related to Tails - Feature #16366: Teach po4a to ignore the [[!meta directives that shall not be translated Confirmed 01/16/2019
Related to Tails - Feature #15673: Import non active languages from Weblate's Git repository into our main tails.git Resolved 06/19/2018
Related to Tails - Feature #15082: Have the Weblate Git communicate with our main Git repository Resolved 03/01/2018
Duplicated by Tails - Bug #16435: Get our ikiwiki features merged upstream Duplicate 03/01/2018

History

#1 Updated by u over 1 year ago

  • Assignee changed from u to hefee
  • Parent task changed from #10034 to #15079

#2 Updated by u over 1 year ago

#3 Updated by u about 1 year ago

Asked intrigeri by email about this today.

#4 Updated by u about 1 year ago

intrigeri gave us an answer via email. We agreed that hefee will look into this before August to make reviewing easy for intrigeri.

We agreed that we will try to fix ikiwiki and intrigeri will review our branch before we ask for a merge upstream: http://source.ikiwiki.branchable.com/?p=source.git;a=blob;f=IkiWiki/Plugin/po.pm;h=418e8e58a2f1a705e8498ad9e22b7afaf8d493ec;hb=HEAD#l95

Introducing a po_disabled_languages setting would be much cleaner and then it can be managed programmatically: e.g. plausibly one could set it empty via a command line argument, and if that does not work it'll be easy to dynamically patch the ikiwiki setup file, while programmatically editing the exclude regexp is crazy.

Now, until this is actually implemented and available in the version of ikiwiki that runs our production website, the exclude option can be a valid temporary solution.

We also have to write config validation and doc for this:

config validation:
http://source.ikiwiki.branchable.com/?p=source.git;a=blob;f=IkiWiki/Plugin/po.pm;h=418e8e58a2f1a705e8498ad9e22b7afaf8d493ec;hb=HEAD#l137

doc:
http://source.ikiwiki.branchable.com/?p=source.git;a=blob;f=doc/plugins/po.mdwn;h=0a764b6947a1f1e307522df46d86d6d6df361ca1;hb=HEAD#l43

#5 Updated by u about 1 year ago

  • Blocks Feature #15673: Import non active languages from Weblate's Git repository into our main tails.git added

#6 Updated by emmapeel about 1 year ago

I see disabling a language affects two things:

- Disabling the language tab in the website (i.e. now we only provide es, fr, de, pt, fa, it)
- Disabling the compilation of the .po files when building the wiki

If we disable the updating and creation of .po files for disabled languages, where do this .po files for disabled languages will be updated?

Right now they are updated when building the wiki, but if we disable them, they are not going to be updated.

And if we enable the compilation, then the wiki will take a lot of time to compile, even for languages we will not enable maybe in 2 or 3 years. This will create a resistance to add new languages just because 'somebody offered to start a translator's team', and raise the bar for the languages in weblate, which will go against the goal of the translation platform (to be able to start translating new languages easily)

I have been making some tests with gitlabCI. Right now when the current wiki on production takes me 10 minutes to compile, the new languages are adding 2 minutes each, and the whole weblate takes 30 minutes to compile, new languages taking in average 2 minutes more (a fifth of the time):

https://0xacab.org/emmapeel/tails/pipelines?scope=branches&page=1

#7 Updated by hefee about 1 year ago

  • Status changed from Confirmed to In Progress
  • Assignee changed from hefee to intrigeri
  • Feature Branch set to https://salsa.debian.org/hefee/ikiwiki/tree/feature/15355-po-plugin-disable-languages

@intrigeri:

I started now to fix ikiwiki po plugin (see feature branch). But as I'm not familiar with the perl I'm do not understand the tests (t/po.t).
  • I'm unable to view/print/dump $links as there are the links to other pages saved, it tells me:
    Variable "$links" is not imported at t/po.t line 147.
    Global symbol "$links" requires explicit package name (did you forget to declare "my $links"?) at t/po.t line 147.
    

    But I can dump $links{'index'}; so why not dump $links; ?
  • I'm missing the generated pages (there is no dest dir in the testdir) and the search of content in the generated files. I think it would help to dump the content of the files so I can understand better what is generated etc.
So far I know, I need to test:
  • de.po is created like all slave languages (done)
  • there is NO de.po.html generated
  • there is NO de.html generated
  • there are NO links from {fr, en, es}.html -> de.html
  • there is NO information, that there is German enabeld on all html pages

#8 Updated by intrigeri about 1 year ago

  • QA Check set to Info Needed

#9 Updated by intrigeri about 1 year ago

  • Assignee changed from intrigeri to hefee
  • I'm unable to view/print/dump $links as there are the links to other pages saved, it tells me:
    > Variable "$links" is not imported at t/po.t line 147.
    > Global symbol "$links" requires explicit package name (did you forget to declare "my $links"?) at t/po.t line 147.
    > 

    But I can dump $links{'index'}; so why not dump $links; ?

To start with, in Perl:

  • $links is a scalar variable.
  • %links is a hash variable.
  • $links{'index'} is the (scalar) value associated to the key 'index' in the %links hash.

So if you want to dump all links, you probably need to pass %links to the dump function:

  • either by value: dump(%links) (which in practice passes a flat list)
  • or by reference: dump(\%links)

The Data::Dumper core module (shipped with Perl by default, think "standard library") has a Dumper function. It produces much better output if you pass data structures by reference as it'll know what kind of structure it is — in this case, a hash — and can thus display it a bit more nicely. So I would do use Data::Dumper; warn Dumper(\%links) or similar :)

  • I'm missing the generated pages (there is no dest dir in the testdir) and the search of content in the generated files. I think it would help to dump the content of the files so I can understand better what is generated etc.

Indeed, these are unit tests for specific parts of the po plugin, not integration tests. You could add the integration test you want if needed. There are a few examples: git grep -E 'refresh|rebuild' -- t :)

So far I know, I need to test:
  • de.po is created like all slave languages (done)

… and deleted/renamed when the "master" page is deleted/renamed via the CGI.

  • there is NO de.po.html generated
  • there is NO de.html generated
  • there are NO links from {fr, en, es}.html -> de.html
  • there is NO information, that there is German enabeld on all html pages

Makes sense to me.

#10 Updated by hefee about 1 year ago

  • Assignee changed from hefee to intrigeri

Yet another issue, if I try to write integration tests (as I don't see a good unit test case).

I tried several ways how to setup the slave/disables languages via commandline but failing:

perl t/po-integration.t
Possible attempt to separate words with commas at t/po-integration.t line 31.
ok 1
{}
"[es|Spanish,de|German]" 
Can't use string ("[de]") as an ARRAY ref while "strict refs" in use at /home/hefee/tails/translation_platform/ikiwiki/IkiWiki/Plugin/po.pm line 194.                                                                              
not ok 2
#   Failed test at t/po-integration.t line 33.
# Tests were run but no plan was declared and done_testing() was not seen.

It looks like perl gets a string expecting a ARRAY and is unhappy about this fact. Can you help here? Do I need to add string parsing?

#11 Updated by intrigeri about 1 year ago

  • Assignee changed from intrigeri to hefee

ikiwiki(1) reads:

       --set var=value
              This allows setting an arbitrary configuration variable, the  same  as
              if  it were set via a setup file. Since most commonly used options can
              be configured using command-line switches, you will rarely need to use
              this.

       --set-yaml var=value
              This is like --set, but it allows setting configuration variables that
              use complex data structures, by passing in a YAML document.

I think you're trying to set "configuration variables that use complex data structures" so I suggest you try using --set-yaml.

#12 Updated by hefee about 1 year ago

  • Assignee changed from hefee to intrigeri
  • % Done changed from 0 to 60
  • QA Check changed from Info Needed to Ready for QA

I tried --set-yaml before but this doesn't accect valid YAML. But I found a way to set the needed data:

--set-yaml po_slave_languages=[es|Spanish,de|German]
--set-yaml po_disabled_languages=[de]

I see one warning:

Smartmatch is experimental at <x>/ikiwiki/IkiWiki/Plugin/po.pm line 285.
Smartmatch is experimental at <x>/ikiwiki/IkiWiki/Plugin/po.pm line 828.
Smartmatch is experimental at <x>/ikiwiki/IkiWiki/Plugin/po.pm line 921.

But anyways, I think it is ready for you to review (keep in mind it is my first perl code). Additionally I was unable to run all tests successfully (but this was the issue before). So I may have triggered error in other tests.

#13 Updated by intrigeri about 1 year ago

  • Assignee changed from intrigeri to hefee
  • QA Check changed from Ready for QA to Dev Needed

I tried --set-yaml before but this doesn't accect valid YAML.

Interesting. Please file a bug report upstream (or link me to the existing one) then.

But I found a way to set the needed data:

> --set-yaml po_slave_languages=[es|Spanish,de|German]
> --set-yaml po_disabled_languages=[de]
> 

Wrt. po_slave_languages: it seems that you're passing a string as the value for a config setting that's supposed to take a list (it still supports a hash for backwards compatibility). I could find the code that parses this string and turns in into the expected $config{po_slave_languages}. Can you please tell me how you've confirmed that this (surprisingly) works, so I can test it?

I see one warning:

> Smartmatch is experimental at <x>/ikiwiki/IkiWiki/Plugin/po.pm line 285.
> Smartmatch is experimental at <x>/ikiwiki/IkiWiki/Plugin/po.pm line 828.
> Smartmatch is experimental at <x>/ikiwiki/IkiWiki/Plugin/po.pm line 921.
> 

Indeed, better not use the smartmatch operator.

But anyways, I think it is ready for you to review (keep in mind it is my first perl code). Additionally I was unable to run all tests successfully (but this was the issue before). So I may have triggered error in other tests.

It would be good to fix that test suite problem. What is it?

First quick review (I'll do a more thorough one later):

  • I see po_disable_languages in some places and po_disabled_languages elsewhere. Please make this consistent.
  • typos: "visiable", "langauges", "enduser"; I stopped looking for typos then, please run a spell checker :)
  • I believe "not visiable in the webpage" and "disable languages in the webpage" do not reflect the implemented behaviour accurately. It's not only that pages in the disabled languages won't be visible, they simply won't be built, right?
  • "languagecodes of the po_slave_languages" is somewhat unclear
  • More generally, user-facing strings could be improved. It would be nice if you got help and reviews from one of your team-mates about this so I can focus on the Perl code.
  • Unless you have a very good reason, let's not introduce a new build-dep on Data::Dump while the rest of the code uses the Data::Dumper core module.

#14 Updated by hefee about 1 year ago

  • Assignee changed from hefee to intrigeri
  • QA Check changed from Dev Needed to Info Needed

intrigeri wrote:

I tried --set-yaml before but this doesn't accect valid YAML.

Interesting. Please file a bug report upstream (or link me to the existing one) then.

I don't know if it is a bug in IkiWiki or YAML module. I thought that I should put a dictinoary to the slave languages:

--set-yaml po_slave_languages={es:Spanish,de:German}
YAML::XS::Load Error: The problem:

    found unexpected ':'

was found at document: 1, line: 1, column: 4
while scanning a plain scalar at line: 1, column: 2

see http://yaml.org/spec/1.2/spec.html#id2759963

Wrt. po_slave_languages: it seems that you're passing a string as the value for a config setting that's supposed to take a list (it still supports a hash for backwards compatibility). I could find the code that parses this string and turns in into the expected $config{po_slave_languages}. Can you please tell me how you've confirmed that this (surprisingly) works, so I can test it?

I dumped the relevant parameters in IkiWiki/Plugin/po.pm:checkconfig():

dump(@{$config{po_slave_languages}});
dump(@slavelanguages);
dump(@{$config{po_disabled_languages}});

Indeed, better not use the smartmatch operator.

What is the good replacement for smarth match operator? I do need a "in list" operator, but search the internet for this I only found solutions, that do a for loop by hand? This is not very handy...

It would be good to fix that test suite problem. What is it?

As I'm new to perl, I may have not a valid environemt to run all tests. At least I know from other languages, that providing a valid testbed is often harder than expected...

The first issue is that I can't run make:

Makefile out-of-date with respect to Makefile.PL
Cleaning current config before rebuilding Makefile...
make -f Makefile.old clean > /dev/null 2>&1
"/usr/bin/perl" Makefile.PL 
Only one of PREFIX or INSTALL_BASE can be given.  Not both.
make: *** [Makefile:920: Makefile] Error 2

so I needed the patch:

--- a/Makefile.PL
+++ b/Makefile.PL
@@ -211,7 +211,7 @@ git-dist:

 WriteMakefile(
        NAME            => 'IkiWiki',
-       PREFIX          => "/usr/local",
+#        PREFIX                => "/usr/local",
        PM_FILTER       => './pm_filter $(PREFIX) $(VER) $(PROBABLE_INST_LIB)',
        MAN1PODS        => {},
        PREREQ_PM       => {

Than I could run make.

$ make test
t/basewiki_brokenlinks.t ....... 3/?
#   Failed test at t/basewiki_brokenlinks.t line 32.

broken links found (with listdirectives)
<li><span class="createlink">inside dot ikiwiki</span> from <a href="./ikiwiki/directive/meta/">meta</a></li></ul>

# Looks like you failed 1 test of 12.
t/basewiki_brokenlinks.t ....... Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/12 subtests
t/relativity.t ................. 1/? # test_site1_perfectly_ordinary_ikiwiki
t/relativity.t ................. 39/? # test_site2_static_content_and_cgi_on_different_servers
t/relativity.t ................. 59/? # test_site3_we_specifically_want_everything_to_be_secure
t/relativity.t ................. 87/? # test_site4_cgi_is_secure_static_content_doesnt_have_to_be
t/relativity.t ................. 131/? # test_site5_w3mmode
t/relativity.t ................. 156/? # test_site6_behind_reverse_proxy
t/relativity.t ................. ok

Test Summary Report
-------------------
t/basewiki_brokenlinks.t     (Wstat: 256 Tests: 12 Failed: 1)
  Failed test:  9
  Non-zero exit status: 1
t/relativity.t               (Wstat: 0 Tests: 177 Failed: 0)
  TODO passed:   36-37, 92
Files=69, Tests=3272, 47 wallclock secs ( 0.51 usr  0.06 sys + 31.09 cusr  4.56 csys = 36.22 CPU)
Result: FAIL
Failed 1/69 test programs. 1/3272 subtests failed.
make: *** [Makefile:965: test_dynamic] Error 255
  • I see po_disable_languages in some places and po_disabled_languages elsewhere. Please make this consistent.

done

  • typos: "visiable", "langauges", "enduser"; I stopped looking for typos then, please run a spell checker :)

done

  • I believe "not visiable in the webpage" and "disable languages in the webpage" do not reflect the implemented behaviour accurately. It's not only that pages in the disabled languages won't be visible, they simply won't be built, right?

well not built yes - But additionaly there is no reference for other languages.

  • "languagecodes of the po_slave_languages" is somewhat unclear
  • More generally, user-facing strings could be improved. It would be nice if you got help and reviews from one of your team-mates about this so I can focus on the Perl code.

I'll ask u to do this.

  • Unless you have a very good reason, let's not introduce a new build-dep on Data::Dump while the rest of the code uses the Data::Dumper core module.

this was only added by accident. There is no need to keep this...

#15 Updated by intrigeri about 1 year ago

  • Assignee changed from intrigeri to hefee
  • QA Check changed from Info Needed to Dev Needed

intrigeri wrote:

I tried --set-yaml before but this doesn't accect valid YAML.

Interesting. Please file a bug report upstream (or link me to the existing one) then.

I don't know if it is a bug in IkiWiki or YAML module. I thought that I should put a dictinoary to the slave languages:

> --set-yaml po_slave_languages={es:Spanish,de:German}
> 

Indeed, that's not valid YAML, you're missing spaces after ":". These work:

  • --set-yaml "po_slave_languages=[es|Spanish, de|German]" (preferred)
  • --set-yaml "po_slave_languages={es: Spanish,de: German}" (backwards compatibility only, as explained previously)

So no bug to report and you're already using the preferred way to set this so we're good.

Wrt. po_slave_languages: it seems that you're passing a string as the value for a config setting that's supposed to take a list (it still supports a hash for backwards compatibility). I could find the code that parses this string and turns in into the expected $config{po_slave_languages}. Can you please tell me how you've confirmed that this (surprisingly) works, so I can test it? […]

Sorry I was confused, you're not passing a string but instead an array, which is correct.

Indeed, better not use the smartmatch operator.

What is the good replacement for smarth match operator? I do need a "in list" operator, but search the internet for this I only found solutions, that do a for loop by hand? This is not very handy...

The grep function is basically a "in list" operator when called in scalar context.

The first issue is that I can't run make:

> Makefile out-of-date with respect to Makefile.PL
> Cleaning current config before rebuilding Makefile...
> make -f Makefile.old clean > /dev/null 2>&1
> "/usr/bin/perl" Makefile.PL 
> Only one of PREFIX or INSTALL_BASE can be given.  Not both.
> make: *** [Makefile:920: Makefile] Error 2
> 

FWIW, in order to be able to run perl Makefile.PL I need to unset PERL_MM_OPT (whose value is INSTALL_BASE=/home/intrigeri/perl5 on my system). I suspect you also have something in your environment that sets INSTALL_BASE or PREFIX.

> Test Summary Report
> -------------------
> t/basewiki_brokenlinks.t     (Wstat: 256 Tests: 12 Failed: 1)
>   Failed test:  9
>   Non-zero exit status: 1
> t/relativity.t               (Wstat: 0 Tests: 177 Failed: 0)
>   TODO passed:   36-37, 92
> Files=69, Tests=3272, 47 wallclock secs ( 0.51 usr  0.06 sys + 31.09 cusr  4.56 csys = 36.22 CPU)
> Result: FAIL
> Failed 1/69 test programs. 1/3272 subtests failed.
> make: *** [Makefile:965: test_dynamic] Error 255
> 

I see the same errors here on upstream master branch so I don't think that's specific to your environment ⇒ please ensure there's a bug report upstream about these issues.

  • I believe "not visiable in the webpage" and "disable languages in the webpage" do not reflect the implemented behaviour accurately. It's not only that pages in the disabled languages won't be visible, they simply won't be built, right?

well not built yes - But additionaly there is no reference for other languages.

OK. So let's please fix this :) I assume u will do it at the same time as:

  • "languagecodes of the po_slave_languages" is somewhat unclear
  • More generally, user-facing strings could be improved. It would be nice if you got help and reviews from one of your team-mates about this so I can focus on the Perl code.

I'll ask u to do this.

… so once the last code problem (smartmatch) is fixed, please reassign to her.

#16 Updated by hefee about 1 year ago

  • Assignee changed from hefee to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:

intrigeri wrote:

I tried --set-yaml before but this doesn't accept valid YAML.

Interesting. Please file a bug report upstream (or link me to the existing one) then.

I don't know if it is a bug in IkiWiki or YAML module. I thought that I should put a dictionary to the slave languages:

[...]

Indeed, that's not valid YAML, you're missing spaces after ":". These work:

  • --set-yaml "po_slave_languages=[es|Spanish, de|German]" (preferred)
  • --set-yaml "po_slave_languages={es: Spanish,de: German}" (backwards compatibility only, as explained previously)

So no bug to report and you're already using the preferred way to set this so we're good.

Sorry but "|" is really no valid YAML. But okay I missed the space after the colon. I had my figth with qw - why I needed to strip spaces... But this is unimportant for this issue.

The grep function is basically a "in list" operator when called in scalar context.

I used the any operator, as it matches better the purpose. Please review again..

FWIW, in order to be able to run perl Makefile.PL I need to unset PERL_MM_OPT (whose value is INSTALL_BASE=/home/intrigeri/perl5 on my system). I suspect you also have something in your environment that sets INSTALL_BASE or PREFIX.

good to know for next time.

I see the same errors here on upstream master branch so I don't think that's specific to your environment ⇒ please ensure there's a bug report upstream about these issues.

I can't create a account for the upstream bugtracker (https://ikiwiki.info/bugs/) as the mailserver is listed as spamsender (branchable.com), so I do not get any mails...

#17 Updated by intrigeri about 1 year ago

Sorry but "|" is really no valid YAML.

Do you mean it's forbidden in a string? ([es|Spanish, de|German] is meant to be a list of strings)

The grep function is basically a "in list" operator when called in scalar context.

I used the any operator, as it matches better the purpose.

Great!

Please review again..

Will do :)

#18 Updated by intrigeri about 1 year ago

  • Assignee changed from intrigeri to hefee
  • QA Check changed from Ready for QA to Dev Needed

I used the any operator, as it matches better the purpose.

Great!

It's obviously nicer than grep but it introduces a new dependency: List::MoreUtils is not in perl core. I don't think it's worth it given s/any/grep/ would work exactly the same in this context (grep might be slower because it counts occurrences; any might be slower because it's implemented in Perl while grep is in C) and this pattern is pretty much idiomatic. Now, if you truly want to use any, I won't block on this; upstream might block (ikiwiki intentionally uses few dependencies, not my preferred approach but well), or might not, YMMV :) But then you need to:

  • check that it's available in t/po.t
  • add it to the list of dependencies in doc/plugins/po.mdwn and debian/control (same as po4a)

This being said, I did a code review and apart of the strings/doc issue that's left to be solved:

  • "will missing the link" ← missing word?
  • Please add a sanity check for the "disabled languages MUST be a subset of slave (sic) languages", otherwise we're deep in unspecified behaviour territory.
  • Did I get the intent correctly that we want to refresh/delete/create the PO files for the disabled languages, but not render them to HTML and never link to them?

Other than that, looks good to me, I don't think I need to do another code review ⇒ feel free to submit upstream once all this has been fixed.

#19 Updated by hefee about 1 year ago

  • Assignee changed from hefee to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

It's obviously nicer than grep but it introduces a new dependency: List::MoreUtils is not in perl core. [snip]

okay just using the grep operator. I come from python were those useful things are included in the core...

This being said, I did a code review and apart of the strings/doc issue that's left to be solved:

  • "will missing the link" ← missing word?

updated wording.

  • Please add a sanity check for the "disabled languages MUST be a subset of slave (sic) languages", otherwise we're deep in unspecified behavior territory.

I added a check and a test for it. I copied the code from https://perlmaven.com/test-for-warnings-in-a-perl-module as Test::Warn would be a new dependency. So I'll request a new code review.

  • Did I get the intent correctly that we want to refresh/delete/create the PO files for the disabled languages, but not render them to HTML and never link to them?

That was my thought, yes. Otherwise we do not gain much with disabled languages over using wiki_file_prune_regexps.

Other than that, looks good to me, I don't think I need to do another code review ⇒ feel free to submit upstream once all this has been fixed.

Should I squash my commits into one commit for presenting the merge request upstream? Is this the correct way to present a patch for upstream?
  1. add my repo to this site: https://ikiwiki.info/git/ ( I do not see how I get my repo added here. Any more information?)
  2. than add a todo via https://ikiwiki.info/todo/ with [[!template id=gitbranch branch=feature/15355-po-plugin-disable-languages author="[[hefee]]"]]
  3. wait for response from upstream...

#20 Updated by intrigeri about 1 year ago

  • Assignee changed from intrigeri to hefee
  • QA Check changed from Ready for QA to Dev Needed
  • Please add a sanity check for the "disabled languages MUST be a subset of slave (sic) languages", otherwise we're deep in unspecified behavior territory.

I added a check and a test for it.
I copied the code from https://perlmaven.com/test-for-warnings-in-a-perl-module as Test::Warn would be a new dependency. So I'll request a new code review.

In "not found in slave languages", please state the exact variable name instead of the ambiguous "slave languages".
Rationale: as a user, I want to be told exactly what I did wrong :)

I'm not convinced that a warning is a good solution: implementation-wise, if this error condition is met, we're in unspecified behavior territory, which I'd rather avoid. Aborting feels safer and it also yields a nicer UX: with a fatal error, I immediately see that I made a mistake, while a warning printed on top of hundreds of lines of output can easily be missed, and then I'm left wondering why things don't behave as I would expect.

  • Did I get the intent correctly that we want to refresh/delete/create the PO files for the disabled languages, but not render them to HTML and never link to them?

That was my thought, yes. Otherwise we do not gain much with disabled languages over using wiki_file_prune_regexps.

OK!

Other than that, looks good to me, I don't think I need to do another code review ⇒ feel free to submit upstream once all this has been fixed.

Should I squash my commits into one commit for presenting the merge request upstream?

Given how the history of the branch looks like, yes, please rebase it a bit. If there are logical changes that can be isolated in separate commits (e.g. the doc change?), please do so though.

Is this the correct way to present a patch for upstream?
  1. add my repo to this site: https://ikiwiki.info/git/ ( I do not see how I get my repo added here. Any more information?)

Last time I checked, clicking the "Edit" button worked fine :)

  1. than add a todo via https://ikiwiki.info/todo/ with [[!template id=gitbranch branch=feature/15355-po-plugin-disable-languages author="[[hefee]]"]]

That template did not exist last time I contributed (or I missed it and just did things in the good old way) so I dunno. What I know is that you need to set the patch tag: https://ikiwiki.info/patch/

#21 Updated by hefee 12 months ago

  • Assignee changed from hefee to u
  • % Done changed from 60 to 80

intrigeri wrote:

In "not found in slave languages", please state the exact variable name instead of the ambiguous "slave languages".
Rationale: as a user, I want to be told exactly what I did wrong :)

done.

I'm not convinced that a warning is a good solution: implementation-wise, if this error condition is met, we're in unspecified behavior territory, which I'd rather avoid. Aborting feels safer and it also yields a nicer UX: with a fatal error, I immediately see that I made a mistake, while a warning printed on top of hundreds of lines of output can easily be missed, and then I'm left wondering why things don't behave as I would expect.

done.

Given how the history of the branch looks like, yes, please rebase it a bit. If there are logical changes that can be isolated in separate commits (e.g. the doc change?), please do so though.

okay - I'll do if we finished this.

I created an upstream TODO for this feature now:
https://ikiwiki.info/todo/_Add_ikiwiki_PO-Plugin_to_ignore_languages

#22 Updated by hefee 12 months ago

@u please look at the descriptions I made around the new feature (WRT comments from intrigeri https://labs.riseup.net/code/issues/15355#note-13).

#23 Updated by u 10 months ago

Looks like hefee needs to rebase this again and later we need to ping upstream.

@intrigeri: because the patch only concerns the po plugin of ikiwiki, and I had in the back of my head that you are actually the author of this plugin, how are chances to have this merged?

#25 Updated by u 10 months ago

  • Assignee changed from u to hefee

#26 Updated by intrigeri 10 months ago

Looks like we need to ping upstream.

Last time I checked, the code (or at least the user-visible strings) was not ready to be submitted upstream, which is why it was assigned to you so you could review and fix those strings. I see no recent change in the Git branch about this sort of things.

At this point, if it's simpler for everyone that I take over this ticket, let's consider this option.

@intrigeri: because the patch only concerns the po plugin of ikiwiki, and I had in the back of my head that you are actually the author of this plugin, how are chances to have this merged?

My Signed-off-by may help but is not sufficient: understandably, the upstream maintainer wants to review and understand the code they'll merge, release and sign.

#27 Updated by intrigeri 10 months ago

intrigeri wrote:

@intrigeri: because the patch only concerns the po plugin of ikiwiki, and I had in the back of my head that you are actually the author of this plugin, how are chances to have this merged?

My Signed-off-by may help but is not sufficient: understandably, the upstream maintainer wants to review and understand the code they'll merge, release and sign.

Sorry, I've read your question too quickly. I think the odds are on our side provided the polishing I've requested is done before submitting upstream. Upstream (smcv) may need months to review & merge but having a good test suite will help.

On our production website we can soon (#14588) use a patched ikiwiki but that's not very useful: every translator, tech writer, and ISO build system would need to use that patched ikiwiki. Not gonna happen.

#28 Updated by intrigeri 10 months ago

And the "patch" tag should be removed on the upstream todo item: our stuff is not ready to be reviewed so pretending it is won't exactly help upstream take us seriously.

#29 Updated by u 10 months ago

Ack, thanks. I've reviewed the strings earlier today, but hefee will integrate it and plish and rebase and then ping upstream again.

#30 Updated by hefee 10 months ago

  • Assignee changed from hefee to u
  • QA Check changed from Dev Needed to Ready for QA
  • removed `patch` tag from upstream
  • integrated u's review
  • rebased & stashed branch

intrigeri wrote:

Sorry, I've read your question too quickly. I think the odds are on our side provided the polishing I've requested is done before submitting upstream. Upstream (smcv) may need months to review & merge but having a good test suite will help.

On our production website we can soon (#14588) use a patched ikiwiki but that's not very useful: every translator, tech writer, and ISO build system would need to use that patched ikiwiki. Not gonna happen.

It is fine, if translators, tech writers, see also the disabled languages, as they may want to update those disabled languages.
We need this feature for ISO build system and the build-website script for the public website.

#31 Updated by u 8 months ago

  • Assignee changed from u to intrigeri

I'm unsure why this is assigned to me. I am not in the position to review this code. Assigning to intrigeri, who is the author of this plugin for review (if budget is accepted).

#32 Updated by intrigeri 8 months ago

  • Assignee deleted (intrigeri)

I'll let someone (possibly our team lead or myself) assign this to me if/once there's budget for it. Meanwhile, I'd rather not have it on my radar :)

#33 Updated by lamby 8 months ago

I actually get test failures here (see attached, not analysed).

#34 Updated by hefee 8 months ago

  • Affected tool set to Translation Platform

@lamby: thanks for starting reviwing this. But this is NOT foundation work, so you need to count it as part of Translations Platform.

According to your build issues - I don't have touched the failing parts only po.t and po-integration.t, so properly something with your env is broken.But don't ask me about Perl - that was my fist perl patch ever in my life.

#35 Updated by intrigeri 8 months ago

  • Blocks Feature #15082: Have the Weblate Git communicate with our main Git repository added

#36 Updated by u 7 months ago

  • Related to Feature #16366: Teach po4a to ignore the [[!meta directives that shall not be translated added

#37 Updated by u 7 months ago

  • Assignee changed from hefee to intrigeri

to be reviewed by intrigeri whenever you decide to do it.

#38 Updated by intrigeri 7 months ago

  • Target version set to Tails_3.15

#39 Updated by intrigeri 5 months ago

  • Target version changed from Tails_3.15 to Tails_3.14

#40 Updated by intrigeri 5 months ago

  • Parent task changed from #15079 to #16435

#41 Updated by intrigeri 4 months ago

  • Feature Branch changed from https://salsa.debian.org/hefee/ikiwiki/tree/feature/15355-po-plugin-disable-languages to https://salsa.debian.org/tails-team/ikiwiki/tree/feature/15355-po-plugin-disable-languages

Rebased on current upstream master, added a couple dozens commits on top, submitted for review upstream: https://ikiwiki.info/todo/_Add_ikiwiki_PO-Plugin_to_ignore_languages/

#42 Updated by intrigeri 4 months ago

  • Target version changed from Tails_3.14 to Tails_3.15
  • QA Check deleted (Ready for QA)
  • Type of work changed from Code to Communicate

I'll ping upstream during next cycle if they did not look at the branch yet. I'll try to follow-up earlier if they review it earlier, if time allows.

#43 Updated by intrigeri 4 months ago

  • Tracker changed from Bug to Feature
  • Subject changed from Fix ikiwiki PO-Plugin to ignore languages to Make the ikiwiki PO plugin able to update PO files for languages that are disabled on the website
  • Description updated (diff)

#44 Updated by intrigeri 4 months ago

  • Estimated time set to 26.00 h
  • Parent task changed from #16435 to #15079

(Getting rid of the #16435 indirection layer, which is only about this very ticket in practice.)

#45 Updated by intrigeri 4 months ago

  • Duplicated by Bug #16435: Get our ikiwiki features merged upstream added

#46 Updated by intrigeri 3 months ago

I've pinged smcv over email.

#47 Updated by intrigeri about 2 months ago

  • Blocks deleted (Feature #15673: Import non active languages from Weblate's Git repository into our main tails.git)

#48 Updated by intrigeri about 2 months ago

  • Related to Feature #15673: Import non active languages from Weblate's Git repository into our main tails.git added

#49 Updated by intrigeri about 2 months ago

  • Blocks deleted (Feature #15082: Have the Weblate Git communicate with our main Git repository)

#50 Updated by intrigeri about 2 months ago

  • Related to Feature #15082: Have the Weblate Git communicate with our main Git repository added

#51 Updated by intrigeri about 2 months ago

  • Target version changed from Tails_3.15 to Tails_3.16
  • Parent task changed from #15079 to #10034

Implemented workarounds so that #15079 is not blocked by this one.

#52 Updated by intrigeri about 2 months ago

Tests failures when trying to build a Debian package based on 3.20190228-1 with our branch imported on top as a quilt patch:

not ok 137 - exactly one error
not ok 138 - checkconfig did not spot configuration error: disabling language that's not enabled

not ok 135 # TODO this should really point back to itself but currently points to example.com
#   Failed (TODO) test at t/relativity.t line 348.
#                   'https://example.com/cgi-bin/ikiwiki.cgi'
#     doesn't match '(?^:^(?:(?:https:)?//staging.example.net)?/cgi-bin/ikiwiki.cgi$)'

Also available in: Atom PDF