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

Relocate python sources into py/ #2249

Merged
merged 10 commits into from
Apr 13, 2022
Merged

Relocate python sources into py/ #2249

merged 10 commits into from
Apr 13, 2022

Conversation

niloc132
Copy link
Member

This also declares wheels as artifacts, making it simpler to declare and install dependencies.

Gradle build/ and python dist/ are now excluded from the python build - if we find this is onerous, buildPyWheel could take a CopySpec argument. With that said, this shouldn't be necessary soon, since deephaven-jpy will hopefully soon be replaced by upstream jpy, and deephaven-legacy will be phased out, leaving only projects that use the new python-wheel plugin.

Legacy deephaven-wheel is renamed now in gradle as well, but not relocated.

Partial #2221

docker/server/build.gradle Outdated Show resolved Hide resolved
py/jpy/build.gradle Outdated Show resolved Hide resolved
@@ -0,0 +1,7 @@
plugins {
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to other reviewers, this replaces py/deephaven2-wheel/build.gradle

@@ -270,11 +255,28 @@ include(':application-mode')
include(':util-immutables')
project(':util-immutables').projectDir = file('Util/util-immutables')

include(':deephaven-jpy')
Copy link
Member Author

Choose a reason for hiding this comment

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

note to other reviewers, I flattened this out since it is a net decrease in actual code, and just as clear as all the other renames we do, but without two different lambdas trying to define these projects

@niloc132
Copy link
Member Author

There are some conflicts waiting for me, as the moved files were modified after I made the commit that relocated them. Ideally I'd like to get a first round of review done before I redo that move, so as to not keep moving the files over and over as changes happen upstream.

@niloc132 niloc132 closed this Apr 11, 2022
@niloc132 niloc132 reopened this Apr 11, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2022
@niloc132 niloc132 force-pushed the dh-from-py branch 2 times, most recently from bcc95fa to 9b022a3 Compare April 12, 2022 16:31
@niloc132 niloc132 marked this pull request as ready for review April 12, 2022 16:35
jmao-denver
jmao-denver previously approved these changes Apr 12, 2022
devinrsmith
devinrsmith previously approved these changes Apr 12, 2022
Comment on lines +16 to +19
COPY wheels/ /wheels
RUN set -eux; \
python -m pip install -q --no-index --no-cache-dir /deephaven-jpy-wheel/*.whl; \
rm -r /deephaven-jpy-wheel

COPY deephaven-wheel/ /deephaven-wheel
RUN set -eux; \
python -m pip install -q --no-index --no-cache-dir /deephaven-wheel/*.whl; \
rm -r /deephaven-wheel

COPY deephaven2-wheel/ /deephaven2-wheel
RUN set -eux; \
python -m pip install -q --no-index --no-cache-dir /deephaven2-wheel/*.whl; \
rm -r /deephaven2-wheel
python -m pip install -q --no-index --no-cache-dir /wheels/*.whl; \
rm -r /wheels
Copy link
Member

Choose a reason for hiding this comment

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

We are "losing" a bit of docker cachability here, but I doubt it's of much consequence.

docker/server/build.gradle Outdated Show resolved Hide resolved
Copy link
Contributor

@jmao-denver jmao-denver left a comment

Choose a reason for hiding this comment

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

LGTM

@devinrsmith devinrsmith self-requested a review April 13, 2022 21:57
@niloc132 niloc132 merged commit 41a27c9 into deephaven:main Apr 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants