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

gui: use existing icons_rc.py #4711

Closed
wants to merge 1 commit into from
Closed

Conversation

toxeus
Copy link
Contributor

@toxeus toxeus commented Sep 13, 2018

This cleans up all sorts of custom build steps or manual user action and is fully compatible with

pip install git+https.

The pyrcc5 command now only needs to be used by developers that wish to update the icons_rc.py
file.

Let's stop pretending that icons_rc.py is not available because since the move to submodules
it is 😉

Contributed by FDF.

@toxeus
Copy link
Contributor Author

toxeus commented Sep 13, 2018

Also, contrary to what this statement, installing using pip install git+https doesn't work for me. According to docs in the pip repo this is expected:

Installing from a local directory or a VCS URL now builds a wheel to install, rather than running setup.py install. Wheels from these sources are not cached. (pypa/pip#4501)

This PR fixes it.

path = str(Path(__file__).parent.joinpath("icons/icons_rc.py"))
spec = importlib.util.spec_from_file_location("icons_rc", path)
module = importlib.util.module_from_spec(spec)
spec.loader.exec_module(module)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By adding __init__.py into the icons submodule it would become a python package and could be imported using a simple import statement. Also, pyinstaller would then automatically pick up that file. If I leave this as is then the *.spec files would need some adjustment. I prefer the icons package solution. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bauerj any opinion on this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Although a package containing only a single module looks wrong I think it does make sense in this context.

setup.py Outdated
@@ -88,6 +69,7 @@ def run(self):
'electrum': [
'wordlist/*.txt',
'locale/*/LC_MESSAGES/electrum.mo',
'gui/qt/icons/icons_rc.py',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This hack can be removed if icons becomes a python package.

print("Please run 'pyrcc5 icons.qrc -o electrum/gui/qt/icons_rc.py'")
print("If you're running electrum from the repo then please make sure that")
print("the submdules are initialized.")
print("git submodule init")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think git submodule update is necessary in this situation too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change it to git submodule update --init.

This cleans up all sorts of custom build steps or
manual user action and is fully compatible with
pip install git+https.

The pyrcc5 command now only needs to be used by
developers that wish to update the icons_rc.py
file.

Let's stop pretending that icons_rc.py is not
available because since the move to submodules
it is ;)
@toxeus
Copy link
Contributor Author

toxeus commented Sep 14, 2018

I have now repointed the icons submodule to point to commit from https:/spesmilo/electrum-icons/pull/2. I'm mostly offline for the next two weeks. If further work on this PR is required then I'm happy to address it when I'm back online.

@SomberNight
Copy link
Member

This would mean the icons submodule needs to be kept up to date with this repo. This is potentially extra maintenance work. With this PR, I guess we should update it every time icons.qrc or icons/ changes.

Currently the electrum-icons repository is only updated just before there is a release. It's only there to make the binaries reproducible. If we could make the built icons_rc.py reproducible, I would much rather we killed the electrum-icons repo.

@toxeus
Copy link
Contributor Author

toxeus commented Sep 14, 2018

This would mean the icons submodule needs to be kept up to date with this repo.

I don't see this to be a hard requirement. You can still keep your workflow as before.

I would much rather we killed the electrum-icons repo.

I like that idea too 👍

@toxeus
Copy link
Contributor Author

toxeus commented Oct 15, 2018

@SomberNight, @bauerj so how do we proceed here?

@toxeus toxeus closed this Oct 18, 2018
@bauerj bauerj reopened this Oct 18, 2018
@toxeus toxeus closed this Jan 11, 2019
mercurytoxic added a commit to mercurytoxic/PKGBUILDs that referenced this pull request Feb 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants