Feature #15355

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

Added by Anonymous 2018-03-01 08:51:44 . Updated 2020-04-22 14:39:37 .

Status:
Rejected
Priority:
Normal
Assignee:
intrigeri
Category:
Internationalization
Target version:
Start date:
2018-03-01
Due date:
% Done:

80%

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
Deliverable for:

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


Files


Subtasks


Related issues

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

History

#1 Updated by Anonymous 2018-03-02 15:36:08

#2 Updated by Anonymous 2018-03-02 15:47:55

#3 Updated by Anonymous 2018-06-12 13:06:29

Asked intrigeri by email about this today.

#4 Updated by Anonymous 2018-06-19 10:46:19

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 Anonymous 2018-06-19 10:56:13

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

#6 Updated by emmapeel 2018-06-22 12:29:22

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 2018-07-08 14:38:44

  • 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 2018-07-08 16:14:38

  • QA Check set to Info Needed

#9 Updated by intrigeri 2018-07-10 12:07:05

  • 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 2018-07-13 23:45:45

  • 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 2018-07-28 05:18:19

  • 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 2018-07-30 15:41:01

  • 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 2018-08-01 03:46:36

  • 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 2018-08-03 08:06:42

  • 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 2018-08-07 06:43:33

  • 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 2018-08-08 10:52:12

  • 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 2018-08-08 11:26:24

> 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 2018-08-09 16:15:15

  • 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 2018-08-10 10:59:58

  • 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 2018-08-13 18:41:54

  • 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?
> # 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 :)

> # 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 2018-08-25 11:07:14

  • Assignee deleted (hefee)
  • % 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 2018-08-25 11:11:17

@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 Anonymous 2018-11-02 17:44:44

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 Anonymous 2018-11-02 18:14:43

  • Assignee set to hefee

#26 Updated by intrigeri 2018-11-02 19:33:55

> 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 2018-11-02 19:42:02

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 (Feature #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 2018-11-02 19:43:06

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 Anonymous 2018-11-02 19:49:24

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 2018-11-07 09:13:12

  • Assignee deleted (hefee)
  • 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 (Feature #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 Anonymous 2018-12-20 12:11:04

  • Assignee set 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 2018-12-20 13:44:44

  • 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 2019-01-06 17:27:38

  • File log.txt added
  • Assignee set to hefee

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

#34 Updated by hefee 2019-01-06 19:19:42

  • 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 2019-01-08 17:11:36

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

#36 Updated by Anonymous 2019-01-17 10:55:55

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

#37 Updated by Anonymous 2019-02-07 15:04:01

  • Assignee changed from hefee to intrigeri

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

#38 Updated by intrigeri 2019-02-08 11:33:55

  • Target version set to Tails_3.15

#39 Updated by intrigeri 2019-03-12 14:40:14

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

#40 Updated by intrigeri 2019-03-12 17:24:24

#41 Updated by intrigeri 2019-04-10 21:03:04

  • 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 2019-04-10 21:04:04

  • 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 2019-04-13 08:31:19

  • 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

#44 Updated by intrigeri 2019-04-13 08:35:57

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

#45 Updated by intrigeri 2019-04-13 08:36:46

  • has duplicate Bug #16435: Get our ikiwiki features merged upstream added

#46 Updated by intrigeri 2019-05-19 16:44:38

I’ve pinged smcv over email.

#47 Updated by intrigeri 2019-06-25 10:46:47

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

#48 Updated by intrigeri 2019-06-25 10:46:51

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

#49 Updated by intrigeri 2019-06-25 10:46:56

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

#50 Updated by intrigeri 2019-06-25 10:47:02

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

#51 Updated by intrigeri 2019-06-25 10:48:07

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

#52 Updated by intrigeri 2019-06-26 15:58:53

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$)'

#53 Updated by intrigeri 2019-08-26 08:15:26

  • Target version changed from Tails_3.16 to Tails_3.17

#54 Updated by intrigeri 2019-09-12 09:21:52

Since we started this work, hefee explained me that even once ikiwiki gets this nice feature, we won’t be able to remove that much (if any) of all the custom Python code we have on top. I don’t recall the detailed explanation. So having this merged upstream will not make that big of a difference.

It would be nice though, so I’m happy to keep pinging upstream about this.

However, I propose we don’t treat this as a blocker for completion of the SponsorS follow-up project anymore, that is: drop the “Deliverable for” field.

#55 Updated by intrigeri 2019-09-12 14:25:09

  • Target version changed from Tails_3.17 to Tails_4.0

#56 Updated by Anonymous 2019-09-13 09:16:47

  • Deliverable for deleted (Sponsor_L)

Agreed.

#57 Updated by intrigeri 2019-09-13 09:30:07

#58 Updated by intrigeri 2019-09-30 15:00:39

  • Target version changed from Tails_4.0 to Tails_4.1

I’ve pinged upstream and will come back to it during next release cycle, to see if smcv replied.

#59 Updated by intrigeri 2019-10-24 08:20:21

  • Target version changed from Tails_4.1 to Tails_4.2

(I don’t want to ping Simon more often than quarterly: I’m pretty sure this would be unhelpful, at best.)

#60 Updated by intrigeri 2019-12-28 09:15:58

  • Target version changed from Tails_4.2 to Tails_4.6

smcv has not committed anything in the upstream Git repo since my last ping, so I don’t think it would be appropriate to ping him now.

#61 Updated by intrigeri 2020-04-22 14:39:37

  • Status changed from In Progress to Rejected
  • Target version deleted (Tails_4.6)

intrigeri wrote:
> Since we started this work, hefee explained me that even once ikiwiki gets this nice feature, we won’t be able to remove that much (if any) of all the custom Python code we have on top. I don’t recall the detailed explanation. So having this merged upstream will not make that big of a difference.
>
> It would be nice though, so I’m happy to keep pinging upstream about this.

7 months later, I’m giving up.