-
Notifications
You must be signed in to change notification settings - Fork 58
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
Fix PyPI Code Deployment workflow #2750
Conversation
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.
__init__
is empty in the wheel - so nothing could be imported from glide.
Same with other 1.2 RCs.
Credits to @tjzhang-BQ for this finding
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.
😌 wheels are not empty now. Can you try running CD on this branch to validate?
node/npm/glide/package.json
Outdated
"copy-declaration-files": "cp ../../build-ts/*.d.ts build-ts/ && cp ../../build-ts/src/*.d.ts build-ts/src/ && cp ../../build-ts/src/server-modules/*.d.ts build-ts/src/server-modules/", | ||
"build": "tsc && mkdir -p build-ts/src && mkdir -p build-ts/src/server-modules && npm run copy-declaration-files" |
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.
This is already added in #2754. Please update your branch.
@@ -250,7 +277,11 @@ jobs: | |||
python -m venv venv | |||
source venv/bin/activate | |||
pip install -U pip | |||
pip install ${PIP_PRE} valkey-glide | |||
if [[ "${{ env.PIP_PRE }}" == "true" ]]; then |
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.
THis should work as it was as I understand. Do you change it because it didn't work or for another reason?
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.
it didnt work
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.
works with release from https://github.com/valkey-io/valkey-glide/actions/runs/12041257002
There seems to be unrelated changes in this PR that we should split out.
@@ -174,10 +177,10 @@ jobs: | |||
if: startsWith(matrix.build.NAMED_OS, 'darwin') | |||
uses: PyO3/maturin-action@v1 | |||
with: | |||
maturin-version: latest | |||
maturin-version: 0.14.17 |
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.
👍
@@ -12,7 +12,7 @@ | |||
ReadFrom, | |||
ServerCredentials, | |||
) | |||
from glide.exceptions import ClosingError, RequestError |
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.
why?
sudo apt-get update | ||
sudo apt-get install -y redis-server | ||
- name: Install engine MacOS |
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.
these commands are under install-engine, no?
@@ -217,6 +220,10 @@ jobs: | |||
matrix: | |||
build: ${{ fromJson(needs.load-platform-matrix.outputs.PLATFORM_MATRIX) }} | |||
steps: | |||
- name: Setup self-hosted runner access | |||
if: ${{ matrix.build.TARGET == 'aarch64-unknown-linux-gnu' }} | |||
run: sudo chown -R $USER:$USER /home/ubuntu/actions-runner/_work/valkey-glide |
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 this cause a permissions issue?
Signed-off-by: Andrew Carbonetto <[email protected]> Signed-off-by: avifenesh <[email protected]>
4bde22c
to
1acc108
Compare
Signed-off-by: Andrew Carbonetto <[email protected]>
By upgrading maturin version in cargo.toml
https://github.com/valkey-io/valkey-glide/actions/runs/11997257172/job/33442732358
Fixed macos imports error by setting maturin version for macos wheels build to 0.14.14, last version where it worked with macos (need to open a bug for maturin)
Fixed macos error:
By setting up all relevant python versions
Avi steps:
Above maturin 0.14.17 the imports breaking in tests pointing on circular dependencies.
The reason is probably that the project folder name (python) is the same name as the source dir, and that the lib (rust code) is the same name as the python module (glide), those leading to this state. Need to refactor the structure, out of scope. ⇒
Managed to find a solution, using a different name in the
cargo.toml
from glide toglide_rs
, and this way can change the rust code to create a module calledglide_rs
.Import it around using
glide_rs
as a separate module instead of.glide,
which is path based and create circular or broken style.Based on reading docs, the issue with mac was solved in 0.15, will run a try pointing to the latest. In case it won't work, will pin on 0.14 until refactor.
My try was wrong, i used lower version for the rc, so the rc it tried to rlease was the older one with higher v.
Will take seconed look tommorow.
We removed support from 3.8 so removed it back.
Simplified the way we use the
--pre
tag so its pass as we want.Issue link
This Pull Request is linked to issue (URL): #2762
Checklist
Before submitting the PR make sure the following are checked: