-
Notifications
You must be signed in to change notification settings - Fork 61
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
dev-qt/qtwebengine-dicts: initial packaging #164
base: master
Are you sure you want to change the base?
Conversation
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should PV
be a timestamp maybe? Like app-portage/eclass-manpages
KEYWORDS="~amd64 ~arm ~arm64 ~x86" | ||
IUSE="" | ||
|
||
S="${WORKDIR}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we usually declare this after the dependencies, although it doesn't really matter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, moved.
|
||
DEPEND=" | ||
app-dicts/myspell-en | ||
dev-qt/qtwebengine" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add slot :5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
case ${lang} in | ||
de-1901) dict="de_1901";; | ||
pt-BR) dict="pt-br";; | ||
*) dict="${lang}";; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation looks weird
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to spaces, the tabs looked deceivingly fine to me.
unset dict lang | ||
|
||
RDEPEND=" | ||
${DEPEND} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can put this all on one line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
bdic_fn=${bdic_fn%.dic}.bdic | ||
bdic_fn=${bdic_fn/_/-} | ||
|
||
"${EPREFIX}"/usr/$(get_libdir)/qt5/bin/qwebengine_convert_dict \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use qt5_get_bindir
from qmake-utils.eclass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
local dicts | ||
local dic | ||
local bdic_fn | ||
local prefix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can declare multiple vars in one line with local
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but I find that makes things less readable. A number of style guides recommend against it in various languages. Still, I'll change them if you insist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I'm fine either way.
Regarding changing PV to a date, I don't think that makes sense here. The eclass-manpages have a date as they're generated from a snapshot of the portage tree. This is using whatever hunspell dictionaries are installed which cannot be directly tied to a specific date (aside from whenever the user ran emerge). Hence I think that the 1.0 is appropriate as this is the 1.0 version of building dictionaries that qtwebengine can use. If the packaging changes in the future, then I'd change the PV at that point. |
I'm aware of that. And technically it's a problem. If the hunspell dicts are updated, the webengine dicts are not, unless the user knows that they have to manually re-emerge |
Also, should there be a dependency on this package from |
Yeah, I don't have a good way to fix that. We can make the PV match the current Qt release as you suggest. Should there also be something in the logs/einfo about it? I don't think that qtwebengine needs a strict dependency but it could be a USE-flag controlled one that defaults to enabled? |
Alternatively, we could see if getting subslots on the dictionaries is a possibility. |
Yeah let's do that please.
Up to you, I'm fine either way.
I thought about that too but it doesn't seem worth the effort. |
Packaging of dictionaries suitable for use by QtWebEngine per https://doc.qt.io/qt-5/qtwebengine-webenginewidgets-spellchecker-example.html. This is simply done by by running qwebengine_convert_dict on each myspell dictionary. Note that: - qt5-build is not used as the dictionaries are not directly dependent upon the version of Qt being used. - Dictionaries are built for every installed myspell dictionary. This could result in a few extra dictionaries if the user is stripping L10N_* flags but that saves complexity in the ebuild as the map from language to dictionary files(s) isn't something that can be simply done with string parsing. See comments in ebuild. Package-Manager: Portage-2.3.13, Repoman-2.3.3
Package-Manager: Portage-2.3.13, Repoman-2.3.3
Packaging of dictionaries suitable for use by QtWebEngine per
https://doc.qt.io/qt-5/qtwebengine-webenginewidgets-spellchecker-example.html.
This is simply done by by running qwebengine_convert_dict on each
myspell dictionary.
Note that:
- qt5-build is not used as the dictionaries are not directly
dependent upon the version of Qt being used.
- Dictionaries are built for every installed myspell dictionary.
This could result in a few extra dictionaries if the user is
stripping L10N_* flags but that saves complexity in the ebuild as
the map from language to dictionary files(s) isn't something that
can be simply done with string parsing. See comments in ebuild.
Package-Manager: Portage-2.3.13, Repoman-2.3.3