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

Got rid of imp rewriting of lib, plugins, and gui dirs #2020

Merged
merged 3 commits into from
Oct 18, 2020
Merged

Conversation

cculianu
Copy link
Collaborator

@cculianu cculianu commented Oct 18, 2020

Module imp is deprecated and will be removed in a future Python
version. So, rather than do the imp rewriting of the lib, gui, and
plugins directories, we renamed these directories to electroncash,
electroncash_gui, and electroncash_plugins to match the Python module
names.

All setup and packaging files have been updated such that Windows,
Linux AppImage, OSX, iOS and srcdist now build correctly using these new
directories.

Android has not been properly migrated over yet, but that will be coming
soon.

Also in this commit: updated Dockerfiles for Wine and Linux AppImage
builds since they grew stale and some package versions needed updating.

We also bumped the app version to 4.2.0 since this filesystem renaming
change can potentially be considedered a significant enough change, as
per semantic versioning.

@cculianu cculianu requested a review from mhsmith October 18, 2020 01:43
@cculianu cculianu added packaging Issues related to building/packaging and not the app itself. WantWantWant Stuff we really want to do but there blocking issues and/or it will take time labels Oct 18, 2020
@cculianu
Copy link
Collaborator Author

@mhsmith or perhaps @jonas-lundqvist or @mpatc -- I wasn't able to correctly update the gradle file(s) -- and I think the Android version no longer builds correctly. Would you mind taking this branch out for a spin and getting the Android version to build correctly and then letting me know what I need to change?

@EchterAgo -- If you also have time and don't mind looking over this PR (I know it's large but most of it is renames) -- just to see if I missed anything. I built AppImage, Windows, OSX, and srcdist and they all work and are correct.

@EchterAgo
Copy link

A few more changes:

EchterAgo@156e95e

@EchterAgo
Copy link

Windows & Linux builds: https://ec.loping.net/4.1.1-32-ge4f688765/

@cculianu
Copy link
Collaborator Author

A few more changes:

EchterAgo@156e95e

Argh sorry I should have done a pull off your branch not a cherry-pick. Can you accept my apology? It was late...

@cculianu
Copy link
Collaborator Author

@EchterAgo Ok so my AppImage has a differeent sha256sum than yours. The Windows EXE's do match, however.

Here is my sha256sum:

fb44981d2b6ee294baea51cf1f9f9cc767c5ed74e940140989d869e96a688023  Electron-Cash-4.1.1-32-ge4f688765-x86_64.AppImage

I noticed that my build log differs from yours in that I got some warnings (looks like something went awry with the CLI args on one of the commands??):

Successfully installed Electron-Cash-4.2.0
You are using pip version 18.1, however version 20.2.4 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
Uninstalling Cython-0.29.7:
  Successfully uninstalled Cython-0.29.7
💬 INFO:  Copying desktop integration
💬 INFO:  Adding launcher
💬 INFO:  Finalizing AppDir
ldd: ./usr/lib/python3.6/site-packages/electroncash_plugins/fusion/Cash: No such file or directory
ldd: ./Fusion: No such file or directory
ldd: ./Logo: No such file or directory
ldd: unrecognized option `-'
Try `ldd --help' for more information.
ldd: ./No: No such file or directory
ldd: ./Text: No such file or directory
ldd: ./Gray.svg: No such file or directory
ldd: ./usr/lib/python3.6/site-packages/electroncash_plugins/fusion/Cash: No such file or directory
ldd: ./Fusion: No such file or directory
ldd: ./Logo: No such file or directory
ldd: unrecognized option `-'
Try `ldd --help' for more information.
ldd: ./No: No such file or directory
ldd: ./Text.svg: No such file or directory
ldd: ./usr/lib/python3.6/site-packages/setuptools/command/launcher: No such file or directory
ldd: ./manifest.xml: No such file or directory
ldd: ./usr/lib/python3.6/site-packages/setuptools/script: No such file or directory
ldd: ./(dev).tmpl: No such file or directory
--- errors end above.. everything below proceeds as normal ---
/lib -> ./lib
/lib/x86_64-linux-gnu -> ./lib/x86_64-linux-gnu
'/lib/x86_64-linux-gnu/libc.so.6' -> './lib/x86_64-linux-gnu/libc.so.6'
--- everything below this line is normal ---

Any thoughts? Note I am building on a macOS host using Docker.

@EchterAgo
Copy link

Doesn't matter, I thought you were going to squash this anyway.

Copy link
Collaborator

@mhsmith mhsmith left a comment

Choose a reason for hiding this comment

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

I assume the Android build problem was this one?

Execution failed for task ':app:mergeMainNetDebugPythonSources'.
> Encountered duplicate path "electroncash_gui/__init__.py" during copy operation configured with DuplicatesStrategy.FAIL

I'll have a look and push a fix to this branch.

cculianu and others added 2 commits October 18, 2020 15:32
Module `imp` is deprecated and will be removed in a future Python
version. So, rather than do the `imp` rewriting of the lib, gui, and
plugins directories, we renamed these directories to electroncash,
electroncash_gui, and electroncash_plugins to match the Python module
names.

All setup and packaging files have been updated such that Windows,
Linux AppImage, OSX, iOS and srcdist now build correctly using these new
directories.

Android has not been properly migrated over yet, but that will be coming
soon.

Also in this commit: updated Dockerfiles for Wine and Linux AppImage
builds since they grew stale and some package versions needed updating.

We also bumped the app version to 4.2.0 since this filesystem renaming
change can potentially be considedered a significant enough change, as
per semantic versioning.
Updated `MANIFEST.in` to include the new package name.

Fixed some comments referring to the old package names.

Removed `package_dir` from `setup.py`, it is not necessary anymore.
@cculianu
Copy link
Collaborator Author

Rebased after migrating the Dockerfile fixes over to master.

@cculianu
Copy link
Collaborator Author

cculianu commented Oct 18, 2020

I assume the Android build problem was this one?

Execution failed for task ':app:mergeMainNetDebugPythonSources'.
> Encountered duplicate path "electroncash_gui/__init__.py" during copy operation configured with DuplicatesStrategy.FAIL

I'll have a look and push a fix to this branch.

@mhsmith

Yes -- and I think it's because now there are 2 copies of electroncash_plugins and electroncash_gui.. so maybe rename one or .. something? No idea. I guess you copy them over so that generate-strings works...?

Thanks man! (Note I just rebased and force-pushed in order to make the Dockerfile fixes for Wine and Linux live in master.. sorry about that!)

@cculianu
Copy link
Collaborator Author

cculianu commented Oct 18, 2020

Regarding the non-reproducibility for AppImage -- @EchterAgo and I traced it down to a bug or quirk in MacOS docker vs Linux docker (I run macOS) -- it is not related to this PR -- it happens off master too. That can be investigated separetely.

@mhsmith
Copy link
Collaborator

mhsmith commented Oct 18, 2020

The Android build works fine now.

@cculianu
Copy link
Collaborator Author

cculianu commented Oct 18, 2020

The Android build works fine now.

Awesome. I'm a little confused -- does this mean it ends up copying all of that code from electroncash_gui and electroncash_plugins into the generated app? Or no?

@mhsmith
Copy link
Collaborator

mhsmith commented Oct 18, 2020

No, because I've changed the include lines to be more specific. It'll now only copy the __init__.py files so the directory is recognized as a package.

@cculianu
Copy link
Collaborator Author

Ah, ok great @mhsmith -- thanks a lot for your rapid help here. Awesome. I'm merging this...

@cculianu cculianu merged commit cab4e37 into master Oct 18, 2020
@cculianu cculianu deleted the fix_dirnames branch October 18, 2020 21:31
cculianu added a commit that referenced this pull request Oct 18, 2020
This script got left behind and was still referring to the old lib/
folder. Fixed. Follow-up to #2020
EchterAgo pushed a commit to EchterAgo/Electron-Cash that referenced this pull request Mar 11, 2021
…follow-up)

This script got left behind and was still referring to the old lib/
folder. Fixed. Follow-up to Electron-Cash#2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packaging Issues related to building/packaging and not the app itself. WantWantWant Stuff we really want to do but there blocking issues and/or it will take time
Projects
None yet
3 participants