Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 'msgstr' is not a valid C format string, unlike 'msgid' #187

Merged
merged 2 commits into from
Feb 8, 2020

Conversation

rbuj
Copy link
Contributor

@rbuj rbuj commented Jan 23, 2020

Closes #185

declare -i LINE

rm po/*.po
tx pull -a

FILE=mate-system-monitor.pot
LINE=$(fgrep -n "msgid \"Width of process '% CPU' column\"" $FILE | cut -d':' -f1)
LINE=$LINE-1
echo "sed -i '"$LINE"d' $FILE" | sh
LINE=$(fgrep -n "msgid \"Show process '% CPU' column on startup\"" $FILE | cut -d':' -f1)
LINE=$LINE-1
echo "sed -i '"$LINE"d' $FILE" | sh

for FILE in po/*.po;
do
  LINE=$(fgrep -n "msgid \"Width of process '% CPU' column\"" $FILE | cut -d':' -f1)
  LINE=$LINE-1
  echo "sed -i '"$LINE"d' $FILE" | sh
  LINE=$(fgrep -n "msgid \"Show process '% CPU' column on startup\"" $FILE | cut -d':' -f1)
  LINE=$LINE-1
  echo "sed -i '"$LINE"d' $FILE" | sh
done

for t in po/*.po; do echo "$(basename "$t" .po)"; done | sort > po/LINGUAS
sed -i '1s/^/# please keep this list sorted alphabetically\n#\n/' po/LINGUAS

Catalan screeshot:

Captura de pantalla a 2020-01-23 09-40-30

@cwendling
Copy link
Member

This doesn't seem like a real fix but a temporary workaround, isn't it? Looks like re-extracting the translations form the gsettings file would break this again, wouldn't it?

@rbuj
Copy link
Contributor Author

rbuj commented Jan 23, 2020

It's just a temporary workaround. Yes, ./makepot will override the fix on pot file, but we should only edit the pot file until someone fixes gettext.

@raveit65
Copy link
Member

This fixes the build but make creates a new resource file po/mate-system-monitor.pot which has the same bad c-format lines.

#: src/org.mate.system-monitor.gschema.xml.in:288
#, c-format
msgid "Width of process '% CPU' column"
msgstr ""

#: src/org.mate.system-monitor.gschema.xml.in:292
#, c-format
msgid "Show process '% CPU' column on startup"
msgstr ""

That means our ./makepot script

# normal translations for the package
make -C po $PACKAGE.pot && mv po/$PACKAGE.pot .

generates the same issue which we push to transifex, and a new pull from transifex breaks building again.

The question is why those entries have the c-format or which part in our code is responsible for this?

@raveit65
Copy link
Member

but we should only edit the pot file until someone fixes gettext.

We can add a sed command to ./makepot to remove c-format from mate-system-monitor.pot, or not?

Than this band aid is good for me , until we find the right cause.

@raveit65
Copy link
Member

raveit65 commented Jan 23, 2020

@rbuj @cwendling
I am fine with adding sed -i "/#, c-format/d" $PACKAGE.pot to makepot script, to avoid this problem when creating tarballs for 1.24 release the next week.

# normal translations for the package
make -C po $PACKAGE.pot && mv po/$PACKAGE.pot .
sed -i "/#, fuzzy/d" $PACKAGE.pot
sed -i "/#, c-format/d" $PACKAGE.pot

@cwendling
Copy link
Member

@raveit65 we probably should not just remove all c-format markers, and definitely not all fuzzy ones.

@raveit65
Copy link
Member

@cwendling
Why?
Any argument?
we remove fuzzy lines to avoid unneeded warnings with older distros.
Please follow other topics where this was decided.

Removing c-format markers is needed to push this shiti things again to transifex.
It doesn't hurt to remove this.
You have all the time to fix this problems in a proper way, but i like to release Mate 1.24 in February.
And i don't have the time to run again in those problems.

@raveit65
Copy link
Member

@cwendling
Please open an report about not removing fuzzy lines where you can discuss this with @sc0w
This isn't related to this PR.

@cwendling
Copy link
Member

Why?
Any argument?
we remove fuzzy lines to avoid unneeded warnings with older distros.

Removing fuzzy translations is fine, but just the fuzzy flag would result in enabling the fuzzy translations, which might be wrong or worse. I admit I didn't follow the other discussions though.

Removing c-format markers is needed to push this shiti things again to transifex.
It doesn't hurt to remove this.

Yes, this is not dangerous, but it prevents Gettext from checking the translated format strings are correct, which could hide nasty bugs. I got bit by that in the past.
However, as a temporary workaround, why not.

@raveit65
Copy link
Member

Removing fuzzy translations is fine, but just the fuzzy flag would result in enabling the fuzzy translations, which might be wrong or worse. I admit I didn't follow the other discussions though.

It is already fixed with gettext-0.20.0 which is used by most distros.
Only debian users will have the pain with that compiling warnings when we remove that line ;)
but this should discussed in a new report with @sc0w

@rbuj rbuj force-pushed the msgstr-error branch 2 times, most recently from c4197e2 to 2920201 Compare January 23, 2020 13:02
@rbuj
Copy link
Contributor Author

rbuj commented Jan 23, 2020

@raveit65 makepot filters-out conflictive c-format comments

example:

[robert@localhost ~]$ cat 2
a

#. makepot: remove comment
#: src/org.mate.system-monitor.gschema.xml.in:33
#, c-format
b

#: src/org.mate.system-monitor.gschema.xml.in:33
#, c-format
c
[robert@localhost ~]$ perl -0777 -p -e 's/#. makepot: remove comment\n#\: src\/org\.mate\.system\-monitor\.gschema\.xml\.in\:([0-9]*)\n#, c-format\n/#\: src\/org\.mate\.system\-monitor\.gschema\.xml\.in\:\1\n/s'  2
a

#: src/org.mate.system-monitor.gschema.xml.in:33
b

#: src/org.mate.system-monitor.gschema.xml.in:33
#, c-format
c

@yetist
Copy link
Member

yetist commented Jan 23, 2020

I see that the line contains "% CPU" just a summary of the configuration items. Maybe we can modify this summary to fix it?

@raveit65 raveit65 self-requested a review January 26, 2020 13:17
Copy link
Member

@raveit65 raveit65 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builds fine and conflicting translations are back.

@raveit65
Copy link
Member

raveit65 commented Jan 26, 2020

I see that the line contains "% CPU" just a summary of the configuration items. Maybe we can modify this summary to fix it?

I will merge this PR for 1.24 release when we don't find another solution.

@lukefromdc
Copy link
Member

I assume you mean 1.24 release, not 1.14 of course

@raveit65 raveit65 merged commit 76d1f15 into master Feb 8, 2020
@raveit65 raveit65 deleted the msgstr-error branch February 8, 2020 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build issue with a lot of languages
5 participants