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

External libraries refactoring #144

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

XavierCLL
Copy link
Collaborator

@XavierCLL XavierCLL commented Apr 1, 2024

  • Refactoring the process of generating (paver) and importing (init) the external libraries required by the plugin because it is not strictly OS-dependent: extlibs_OS > extlibs
  • Now is enough to have a Github action for only one OS
  • Fixes some paver packaging

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 ? )

Test Windows Linux Mac
GEE Authentication ✅ Xavier
tester 2
✅ Xavier
tester 2
✅ SiggyF
tester 2
GEE Example ✅ Xavier
tester 2
✅ Xavier
tester 2
✅ SiggyF
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)

Copy link
Collaborator

@SiggyF SiggyF left a 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

requirements.txt Outdated Show resolved Hide resolved
"media",
"ee_plugin.zip"
]
),
)


def clean_extlibs():
Copy link
Collaborator

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?

Copy link
Collaborator Author

@XavierCLL XavierCLL May 25, 2024

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

@SiggyF
Copy link
Collaborator

SiggyF commented May 25, 2024

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:

  • crawling and removing files feels a bit uncomfortable.
  • the earthengine version is now pinned, I added a comment to revert that.
  • It's named linux, while it's now all/any. The package is now (probably was already) double zipped.

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.

@XavierCLL
Copy link
Collaborator Author

The file is now double zipped with linux in the name: name: ee_plugin-${{ matrix.os }}.zip

That is a Github action issue/limitation

@XavierCLL
Copy link
Collaborator Author

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:

* crawling and removing files feels a bit uncomfortable.

* the earthengine version is now pinned, I added a comment to revert that.

* It's named linux, while it's now all/any.  The package is now (probably was already) double zipped.

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.

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!
Xavier

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants