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

Cs/sc 3351 compile det exe linux #236

Merged
merged 6 commits into from Mar 25, 2024
Merged

Conversation

Charl1996
Copy link
Contributor

@Charl1996 Charl1996 commented Mar 7, 2024

Ticket

This PR broadens the DET horizons by introducing functionality to compile an executable file of the DET (currently only compatible with Linux, Windows coming soon).

Compiling executable files from a python project can be done using pyinstaller.

The executable file are compiled automatically on every release publish and will be added as assets to the release once the workflow completes. Any person can then simply download the file from there and use it (through the CLI) like you would as if you installed commcare-export,

./commcare-export < options >

Why do this?

The big push for this was the fact that some folks using the DET is less technically-inclined and having to manage a python codebase and it's packages poses a barrier to usage. Now users can simply download and run the file without thinking about any installation steps. That said, most users use Windows machines, so this PR, although it does add value bringing it to Linux, can be seen more or less laying the foundation for Windows compilation. (I decided to split the Windows compilation into another ticket since it's a slightly bigger lift).

Notes

I added a requirements.txt file to install the necessary packages (those specified by setup.py's extras_required) not installed when simlpy running

pip install commcare-export

Known issues

One known issue is, if you run

./commcare-export --version

it says the version is "None". This is probably due to these lines (which I added, otherwise an error message says that git is not installed). I think the real issue is why the version cannot be deduced from the stored_version.

@Charl1996 Charl1996 marked this pull request as ready for review March 7, 2024 13:18
@@ -17,6 +17,9 @@ def stored_version():


def git_version():
if os.environ.get('DET_EXECUTABLE'):
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 was added so that running the binary will not output an error saying "git was not found". It's expected that folks using the binary might not have git installed so deducing the version from git as is done in this function is redundant for the bundled environment.

Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be an issue for built versions of the tools since we write the version to a file and use that in preference to the git version.

@@ -8,7 +8,7 @@ set -e

cd /src

pip install .
pip install commcare-export
Copy link
Contributor Author

@Charl1996 Charl1996 Mar 7, 2024

Choose a reason for hiding this comment

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

There was an issue where the version is not set, so doing

pip install .

don't work; maybe something to look into in the future. Not sure how important it is right now since we're able to simply run pip install commcare-export (of course, running pip install commcare-export assumes the newest version is already pushed to pypi, which at this stage during the release process should be true).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need -e. And depending on the version of Python, you might need setuptools too:

pip install setuptools
pip install -e .

Disclaimer: I have not tested this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I tried it, but alas...it didn't work.

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 tried various other ways which was also a dead end or just unfeasible in the long run...I don't think this issue is really blocking, just annoying (let me know if you disagree), so I'll create a ticket to address it.

Copy link
Contributor

@SmittieC SmittieC left a comment

Choose a reason for hiding this comment

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

Nice. Two thoughts:

  1. Regarding the version, maybe there's a better solution to my suggestion, but could we not set it as an environment variable in the runtime_hook.py file, since we have git tools, and then just read/show it when DET_EXECUTABLE is active?
  2. I'm wondering about a world where all the build related files is contained within the image, so the commcare export repo is kept "clean" from those files. Just a thought

@Charl1996
Copy link
Contributor Author

@SmittieC

  1. We could, although I'd still want to check out why the "standard way" of checking it does not work.
  2. I envisioned the build_exe directory more as a place where we keep a versioned history of the files needed to build the image, so even if we theoretically push all those files into an image we'd still have to keep some sort of record of it somewhere in case we want to change the image. Or do you mean something else?

@SmittieC
Copy link
Contributor

SmittieC commented Mar 8, 2024

  1. I envisioned the build_exe directory more as a place where we keep a versioned history of the files needed to build the image, so even if we theoretically push all those files into an image we'd still have to keep some sort of record of it somewhere in case we want to change the image.

Hmm yeah it's probably fine this way. We'd have to keep track of these files somewhere. Might as well be this repo

@SmittieC
Copy link
Contributor

SmittieC commented Mar 8, 2024

We could, although I'd still want to check out why the "standard way" of checking it does not work.

Are you planning on figuring this out for this PR still?

@Charl1996
Copy link
Contributor Author

Charl1996 commented Mar 8, 2024

Are you planning on figuring this out for this PR still?

Not right now, I first want to continue with other sprint work. If there's time and this PR is still open I'll do that as part of this PR (otherwise definitely in the near future). I also want to hear from other folks what they think the impact is of not knowing the version (except the annoyance of users trying to figure out what version they're running). In essence, I'm not sure if the version issue is a blocker for this PR.

@Charl1996 Charl1996 merged commit a679618 into master Mar 25, 2024
5 checks passed
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

4 participants