-
-
Notifications
You must be signed in to change notification settings - Fork 117
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
External libraries refactoring #144
Conversation
…external libraries required by the plugin because is not strictly OS-dependent
…es in this extlibs refactoring
…sults, otherwise not possible to test it because a bug #133
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.
The file is now double zipped with linux in the name:
name: ee_plugin-${{ matrix.os }}.zip
"media", | ||
"ee_plugin.zip" | ||
] | ||
), | ||
) | ||
|
||
|
||
def clean_extlibs(): |
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.
Can we do without this? I find it a bit scary to do an os.walk
+ os.remove
on users computers.
Or perhaps remove the specific directory?
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.
After some testing, I realized that the dependencies of this plugin are not OS-depend, you don't want to have windows DLLs in your MacOS plugin dir and so on (to be honest, there are pretty few linked files). So this deletes any unneeded linked lib and caches. Beside:
- This function runs only in the ext_lib directory created when pave is call
- This function is part of the plugin packaging tool that runs in a docker container inside Github action, in theory a user never needs to run this
- Another purpose of this is to reduce the plugin size
I think the pycrypto library was the only native (c-based) dependency (which also required manual compilation). If that's no longer needed, we can see the distribution as an "all" / "any" / "python only" distribution. I left a few minor comments:
You could also consider keeping it as is. Once there will be a system specific dependency added back, we need to revert back to this approach. I tested it, and it works on my m1 mac. |
That is a Github action issue/limitation |
Hi @SiggyF thanks so much for your review! As soon as I realized that EE API no longer needed pycrypto, I tested and created this PR to have an independent OS for the external libs for the plugin. And, of course, if in the future the EE-API includes a library, not OS-independent, we can roll back to the old code. The good thing to have this, simplified and not OS-depend: is easy to maintain and release it, a lot of size reduction for the final plugin package, fewer GitHub actions. Regards! |
@@ -1,5 +1,4 @@ | |||
ratelim | |||
pyCrypto | |||
earthengine-api | |||
earthengine-api>=0.1.335 |
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.
Please change this to a specific version to avoid mismatch between EE versions for users, which will be harder to debug in case of bugs. For example, 1.20 would be good here.
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.
I think that it is not necessary due that those libs are distributed inside the final package (ran by the github action inside the docker), so users never install it directly. But, before the next release we need to update the "extlibs" and set up the latest versions of those libs
With this, we are going to reduce the complexity of generating and packaging the external libraries (and package size). We need to check the new extlibs through two tests ( @gena and @SiggyF ? )
tester 2
tester 2
tester 2
tester 2
tester 2
tester 2
For testing, use the Artifact at https://github.com/gee-community/qgis-earthengine-plugin/actions/runs/8509882708 (the package is created by Ubuntu OS but should work for any OS)