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

Python3 fixes for storage api #5453

Merged
merged 4 commits into from
May 22, 2024
Merged

Conversation

ydirson
Copy link
Contributor

@ydirson ydirson commented Feb 13, 2024

This is a remake of #5375

It limits messing with the builtins, by only defining on py3 the missing long and unicode as aliases where needed.

@psafont
Copy link
Member

psafont commented Feb 13, 2024

We're in a freeze period, and seeing how porting python code has been risky later, we've prepared a python3 branch where this can be safely merged to, I'm changing the base branch so it can be promptly merged.

@psafont psafont changed the base branch from master to feature/py3 February 13, 2024 10:20
@psafont
Copy link
Member

psafont commented Feb 13, 2024

Ah, this brings a lot of commits into the base branch because it's quite out-of date, I've opened #5454 to merge all the commits independently of your changes

@ydirson
Copy link
Contributor Author

ydirson commented Feb 21, 2024

Hm, I don't see why those 4 commits based on v24.5.0, included in feature/py3, is seen as merging 41 commits:

$ git log --oneline --no-decorate  origin/feature/py3..xcpng/storage-api-py3 
eb0210b70 Remove now-unused PY_TEST guard
9acbbdbcc Switch xapi-storage-scripts tests to python3
473d34b30 py3: make sure we are not using unicode type in python3
3791d8954 py3: make xapi-storage py3-compatible

@psafont
Copy link
Member

psafont commented Feb 21, 2024

This is because the PR was created before the feature/py3 branch included the commits

maybe a rebase can work around this github issue?

This is a redo of 4140ff1, not touching
`str`.

This uses the same mechanism as ac683ca,
to deal with occurrence of `long` causing `make test` to fail with
python3, but also occurrences of `unicode`.

Signed-off-by: Yann Dirson <yann.dirson@vates.fr>
This is a redo of 48c8c3e, not
touching `long` and `str`.

`unicode` is only used in there for testing whether we have a string,
so aliasing it to `str` is valid.  OTOH we likely don't want to accept
`bytes` where we accept `str` in python2, and `str` gets aliased to
`bytes` in other areas of the code, so this might reveal issues in
other places.

Signed-off-by: Yann Dirson <yann.dirson@vates.fr>
With this a "make test" after build out of OPAM on Debian 12 finishes
successfully.

Signed-off-by: Yann Dirson <yann.dirson@vates.fr>
Reported-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
Signed-off-by: Yann Dirson <yann.dirson@vates.fr>
Copy link

codecov bot commented Feb 23, 2024

Codecov Report

Merging #5453 (af3a3e4) into feature/py3 (13258e5) will increase coverage by 6.7%.
Report is 7 commits behind head on feature/py3.
The diff coverage is n/a.

Additional details and impacted files
@@              Coverage Diff              @@
##           feature/py3   #5453     +/-   ##
=============================================
+ Coverage         49.0%   55.8%   +6.7%     
=============================================
  Files               18      15      -3     
  Lines             2319    2032    -287     
=============================================
- Hits              1138    1135      -3     
+ Misses            1181     897    -284     

see 5 files with indirect coverage changes

Flag Coverage Δ
python2.7 ?
python3.11 55.8% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@psafont
Copy link
Member

psafont commented Feb 23, 2024

Codecov doesn't seem to make sense here, this PR did not change scripts/ .

I wonder whether coverage could be calculated when running make test, or dune build @runtest-python. Any ideas, @bernhardkaindl ?

Copy link
Collaborator

@bernhardkaindl bernhardkaindl left a comment

Choose a reason for hiding this comment

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

@ydirson / @liulinC : Regarding the failed coverage check in this PR:

---> Rebasing it to the tip of the target branch feature/py3 should help.

Because it's targeted at the branch feature/py3, I add an approval for testing it there!

Regarding coverage, I'm copying this here from #5511 (as Pau asked if it is possible to cover it):

The xapi-storage Python script self-tests are started from
ocaml/xapi-storage-script/main.ml, and I do not understand it good enough to see how to make changes to invoke the coverage module instead of the script.

python -m coverage run scripts/examples/python/shell.py

Pau found: https://coverage.readthedocs.io/en/7.4.4/subprocess.html

Create or append to sitecustomize.py to add these lines:

    import coverage
    coverage.process_startup()

Create a .pth file in your Python installation containing:

    import coverage; coverage.process_startup()

@liulinC
Copy link
Collaborator

liulinC commented Mar 18, 2024

8a72e0f introduced a similar updates, and 1d05a6f revert it.
From the comments above, I understand it to de-risk XS8 GA.

But,

  • has the PR been tested in a live XS, as we all know it is tricky to update py2 to py3, and just static checker are NOT good enough.
  • Can override builtins be avoided, it will confuse others, especially they do not have such context.

@psafont
Copy link
Member

psafont commented Apr 15, 2024

@liulinC can you have a look into how to progress this conversion to python 3? I believe it's worthwhile that the conversion team at the very least tracks progress of this in a ticket so it can be progressed however it's convenient.

@liulinC
Copy link
Collaborator

liulinC commented May 21, 2024

this conversion to pyth

hi @psafont Sorry I just found message.
There are some commit to revert similar changes:

May I know the background of such reverts? It looks very likely revert your revert. (Not exactly, but part of it. Considering you already approved, just double confirm.)

@psafont
Copy link
Member

psafont commented May 21, 2024

The previous changes was not a correct conversion and broke the product. This was detected in internal testing.

@liulinC liulinC merged commit 6226bbd into xapi-project:feature/py3 May 22, 2024
18 of 19 checks passed
@liulinC
Copy link
Collaborator

liulinC commented May 22, 2024

The previous changes was not a correct conversion and broke the product. This was detected in internal testing.

Thanks, merge now for further test.

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.

4 participants