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

app-text/sigil: version bump to 0.9.13 #11264

Closed
wants to merge 2 commits into from

Conversation

arthurzam
Copy link
Member

Please note that the package needs a new maintainer, based on what this page shows, and I will gladly take over as a Proxy Maintainer - but I don't know the procedures.

@gentoo-bot
Copy link

Copyright policy change

Please note that on 2018-09-15 Trustees have approved new Gentoo copyright policy. All contributions made to Gentoo need to follow this policy. If you include the Signed-off-by line in your commit message, you indicate that you have read the policy and agree to its terms. For more detailed explanation, please see the new Gentoo copyright policy explained article.

Pull Request assignment

Submitter: @arthurzam
Areas affected: ebuilds
Packages affected: app-text/sigil

app-text/sigil: @gentoo/proxy-maint (maintainer needed)

Linked bugs

No bugs to link found. If your pull request references any of the Gentoo bug reports, please add appropriate GLEP 66 tags to the commit message and request reassignment.


In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of ConductCopyright policy (expl.) ● DevmanualGitHub PRsProxy-maint guide

@gentoo-bot gentoo-bot added maintainer-needed There is at least one affected package with no maintainer. Review it if you can. assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. labels Mar 5, 2019
app-text/sigil/sigil-0.9.12.ebuild Outdated Show resolved Hide resolved
app-text/sigil/sigil-0.9.12.ebuild Outdated Show resolved Hide resolved
app-text/sigil/sigil-0.9.12.ebuild Outdated Show resolved Hide resolved
app-text/sigil/sigil-0.9.12.ebuild Outdated Show resolved Hide resolved
app-text/sigil/sigil-0.9.12.ebuild Outdated Show resolved Hide resolved
app-text/sigil/sigil-0.9.12.ebuild Outdated Show resolved Hide resolved
@juippis
Copy link
Member

juippis commented Mar 5, 2019

I will gladly take over as a Proxy Maintainer - but I don't know the procedures.

Edit the metadata.xml file, adding yourself and the proxy-maint project there. See example from already proxy-maintained packages :)

You need a bugzilla account with that email you add to the metadata.

@arthurzam
Copy link
Member Author

@juippis Is it better to force push or create a new commit in this pull request?

@juippis
Copy link
Member

juippis commented Mar 5, 2019

If you have the knowledge to rebase and force-push, do that please.

So fix the ebuild, make a new commit, rebase and squash and that commit into your first commit so there is only one commit with version bump, then make a new commit updating metadata.xml file.
So in the end there are 2 commits, one with version bump, one with metadata.xml update.

@arthurzam
Copy link
Member Author

@juippis Big thank you for your help!

app-text/sigil/sigil-0.9.12.ebuild Outdated Show resolved Hide resolved
app-text/sigil/sigil-0.9.12.ebuild Outdated Show resolved Hide resolved
app-text/sigil/sigil-0.9.12.ebuild Outdated Show resolved Hide resolved
@juippis
Copy link
Member

juippis commented Mar 5, 2019

Sorry I missed few things in the initial round, but overall it's looking good!

EDIT: The error above seems to be from dev-python/cssutils not being updated for python3.7 yet. You can update this ebuild to be compatible with python3.6 max, or investigate, test and update cssutils to support python3.7 before this gets merged. You can put the python3.7 bump to this PR, or make a new PR for that. You can also wait for this to get merged with python3.6, then make a new PR to update cssutils, and after that is merged, make a new PR to add python3.7 to this package. That's probably the best way.

@arthurzam
Copy link
Member Author

Sorry I missed few things in the initial round, but overall it's looking good!

EDIT: The error above seems to be from dev-python/cssutils not being updated for python3.7 yet. You can update this ebuild to be compatible with python3.6 max, or investigate, test and update cssutils to support python3.7 before this gets merged. You can put the python3.7 bump to this PR, or make a new PR for that. You can also wait for this to get merged with python3.6, then make a new PR to update cssutils, and after that is merged, make a new PR to add python3.7 to this package. That's probably the best way.

Thank for your comments.
I put a bug in bugzilla. Is it better to put a pull request?
About the version check, after looking in dev-util/electron ebuild, I found the ver_test from EAPI 7, which I think is better.

@juippis
Copy link
Member

juippis commented Mar 5, 2019

I put a bug in bugzilla. Is it better to put a pull request?

It's good, the python project should be pretty active both in bugzilla and Github. Whatever is best for you, as long as you can track the progress. EDIT: Pull request might speed it up a little bit, but as said, python project should be pretty active with these things.

About the version check, after looking in dev-util/electron ebuild, I found the ver_test from EAPI 7, which I think is better.

Yes it should work too. You can choose the implementation yourself, whatever you find easier and more to your liking. Bear in mind, using ver_test still requires you to inherit toolchain-funcs for gcc-version.

@arthurzam arthurzam changed the title app-text/sigil: version bump to 0.9.12 app-text/sigil: version bump to 0.9.13 Mar 21, 2019
@arthurzam
Copy link
Member Author

The problem with dev-python/cssutils python3.7 support was fixed.

Upstream version was updated to 0.9.13 - updated this PR.
Added missing call for xdg_desktop_database_update from xdg-utils.eclass
Moved dev-qt/linguist-tools:5 into BDEPEND, as done in other Qt projects.
Changed SRC_URI to use .tar.gz - removed app-arch/unzip dependency.

@arthurzam
Copy link
Member Author

@gentoo/proxy-maint Could someone check and maybe merge it?

app-text/sigil/sigil-0.9.13.ebuild Outdated Show resolved Hide resolved
app-text/sigil/sigil-0.9.13.ebuild Outdated Show resolved Hide resolved
Signed-off-by: Zamarin Arthur <arthurzam@gmail.com>
@arthurzam
Copy link
Member Author

  • Removed bundled in package mathjax (now depends on dev-libs/mathjax)
  • Removed bundled hunspell dictionaries
  • Added readme.gentoo-r1
  • Changed the install patch to use GNUInstallDirs. Spoke with maintainer about including the patch upstream, they are against because they are afraid to break other distribution's packages. But they said they are planning to integrate a way to select at least lib -> lib64 (which is what we need for this package).

Another question:
The package includes some python modules for sigil. How (and if) should I python_optimize (I saw it in some other ebuilds but haven't understood how)?

@mgorny
Copy link
Member

mgorny commented Apr 26, 2019

You can find eclass documentation either inside the files, in eclass-manpages package or in devmanual.

@mgorny
Copy link
Member

mgorny commented Apr 26, 2019

Oh, and python eclasses are also documented in detail on wiki, in Project:Python.

@arthurzam
Copy link
Member Author

arthurzam commented Apr 26, 2019

@mgorny I hope I have done it right (no that sure about python_optimize - mainly about the passed argument).
Also fixed license (internal gumbo is licensed under Apache-2). Is it correct set in ebuild?

app-text/sigil/files/sigil-0.9.13-fix-install.patch Outdated Show resolved Hide resolved
app-text/sigil/sigil-0.9.13.ebuild Outdated Show resolved Hide resolved
app-text/sigil/sigil-0.9.13.ebuild Outdated Show resolved Hide resolved
app-text/sigil/sigil-0.9.13.ebuild Outdated Show resolved Hide resolved
@arthurzam
Copy link
Member Author

@mgorny Done.
Never thought about the download name and extracted name. Everyday you learn something new and interesting :)

@mgorny
Copy link
Member

mgorny commented Apr 27, 2019

Thanks. Please give me some time to build qtwebkit.

@mgorny
Copy link
Member

mgorny commented Apr 27, 2019

You seem to be missing some deps still:

Traceback (most recent call last):

  File "/usr/share/sigil/python3lib/xmlprocessor.py", line 6, in <module>
    from sigil_bs4.builder._lxml import LXMLTreeBuilderForXML

  File "/usr/share/sigil/plugin_launchers/python/sigil_bs4/builder/_lxml.py", line 26, in <module>
    from lxml import etree

ImportError: No module named 'lxml'

@arthurzam
Copy link
Member Author

You seem to be missing some deps still:

Traceback (most recent call last):

  File "/usr/share/sigil/python3lib/xmlprocessor.py", line 6, in <module>
    from sigil_bs4.builder._lxml import LXMLTreeBuilderForXML

  File "/usr/share/sigil/plugin_launchers/python/sigil_bs4/builder/_lxml.py", line 26, in <module>
    from lxml import etree

ImportError: No module named 'lxml'

Interesting, as dev-python/lxml[${PYTHON_USEDEP}] is a dependency.

@mgorny
Copy link
Member

mgorny commented Apr 27, 2019

Ok, there's something really wrong going on here:

 0x0000000000000001 (NEEDED)             Shared library: [libpython3.4m.so.1.0]

So it's ignoring PYTHON_SINGLE_TARGET completely.

@arthurzam
Copy link
Member Author

@mgorny Well, that is really weird as I haven't touched anything about the python usage. I guess it came from the previous ebuilds. I think I know where to check and fix it. Will take some time.

@arthurzam
Copy link
Member Author

@mgorny I hope I fixed (but I ask you to recheck it).
I added a call to python_fix_shebang.
Somehow until now the check in cmake worked with some python wrapper magic (well I have no idea how but it found somehow the correct version). But to be sure I now explicitly pass as an argument the correct python executable.

@mgorny
Copy link
Member

mgorny commented Apr 29, 2019

There's still something wrong with it:

-- Found PythonInterp: /usr/bin/python3.7 (found suitable version "3.7.3", minimum required is "3.4") 
-- Found PythonLibs: /usr/lib64/libpython3.4m.so (found suitable version "3.4.10", minimum required is "3.4") 

app-text:sigil-0.9.13:20190429-132026.log

I don't see anything useful in CMakeOutput.txt.

Closes: https://bugs.gentoo.org/682700
Package-Manager: Portage-2.3.65, Repoman-2.3.12
Signed-off-by: Zamarin Arthur <arthurzam@gmail.com>
@arthurzam
Copy link
Member Author

@mgorny First of all, sorry for all this trouble. I sincerely am apologizing for wasting your time.
I found the culprit! In this line, if python3.4 is installed it wins the previous configuration of using PYTHON_EXECUTABLE.
Haven't found this bug previously as I don't have installed python3.4 and I tries various changes with 3.5 and 3.6 (main system uses 3.7). I managed to reproduce your report by installing python3.4.
Fixed with passing all needed python configurations.
I sorry again to ask it, but please check it again.

@mgorny
Copy link
Member

mgorny commented Apr 29, 2019

No need to apologize. Cool you found the culprit. FWICS it built fine this time and runs, so I'll merge it. Thanks!

@mgorny
Copy link
Member

mgorny commented Apr 29, 2019

For the future, I'd suggest:

set directory=~/tmp,/var/tmp

This will stop vim from making .swp files in current directory, and you from committing them ;-).

@arthurzam arthurzam deleted the app-text/sigil branch April 29, 2019 14:31
@arthurzam arthurzam restored the app-text/sigil branch April 29, 2019 14:32
@arthurzam arthurzam deleted the app-text/sigil branch April 29, 2019 14:32
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2019-04-29 14:42 UTC
Newest commit scanned: 6d4dc71
Status: ✅ good

No issues found

@eli-schwartz
Copy link
Contributor

Changed the install patch to use GNUInstallDirs. Spoke with maintainer about including the patch upstream, they are against because they are afraid to break other distribution's packages. But they said they are planning to integrate a way to select at least lib -> lib64 (which is what we need for this package).

(This is now merged to master, with a suitable changelog message.)

BTW a bunch of these modules have never been a hard dep (in my Arch packaging I use an optional dependency for many of the python modules) but cssutils has been essentially dropped in favor of css-parser. I've confirmed with Doug and Kevin that for my distro packaging purposes it's fine to rm -r src/Resource_Files/plugin_launchers/python/css_parser/ and depend on the python-css-parser package instead -- you may want to do the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s). maintainer-needed There is at least one affected package with no maintainer. Review it if you can. no bug found No Bug/Closes found in the commits.
Projects
None yet
6 participants