-
Notifications
You must be signed in to change notification settings - Fork 182
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
Try to improve the Azure pipeline, and publish the intermediate wheels for debugging #1233
Try to improve the Azure pipeline, and publish the intermediate wheels for debugging #1233
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1233 +/- ##
=======================================
Coverage 87.16% 87.16%
=======================================
Files 113 113
Lines 10287 10287
Branches 4045 4045
=======================================
Hits 8967 8967
Misses 727 727
Partials 593 593 ☔ View full report in Codecov by Sentry. |
f58921e
to
8d1a229
Compare
8d1a229
to
8118622
Compare
Somehow I've triggered the errors reported in #1178.
(python 3.12+) |
14d8f31
to
09c6ebe
Compare
displayName: 'Install dependencies' | ||
|
||
- script: | | ||
python -m pip wheel . -w wheelhouse/ | ||
displayName: 'Build wheel' | ||
inputs: |
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.
Sadly this is only done on release. It would be good to validate against a wheel in our systematic testing.
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.
@Thrameos - I've drawn a blank as to why there would be no org.jpype.jar
file in the wheels.
The fact that we don't test wheels in the CI is a concern - it means we find out about this in the release, and I suggest we do something about that (e.g. remove editable install testing for anything other than coverage tests, and replace with wheel based testing).
Locally, I was unable to produce a wheel with the JAR from a clean build. Perhaps there is something about the way that the repo is checked out, perhaps along with the fact that org.jpype.jar
is not explicitly mentioned in the MANIFEST.in
.
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.
In the previous system we used setupext to trigger a custom logics in the sdist which causes the jar to be built and included. Without this logic it may just create a bogus sdist package.
We don't build the jar on the users system because it causes way too many issues when people have jar built with random Java versions. Some builds with a later versions then distributes it to another user with an earlier version and we get version errors that weren't tied to anything in our release. Thus jar must be shipped.
@@ -44,4 +44,3 @@ sudo ln -s /usr/local/bin/python$PYTHON_VERSION /usr/local/bin/python | |||
which python | |||
python --version | |||
python -m ensurepip | |||
python -m pip install setuptools wheel |
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 isn't needed since it is only used in the release.yml, and that uses pip
, which install the necessary build deps.
@@ -11,7 +11,7 @@ steps: | |||
- script: | | |||
python setup.py test_java | |||
pip install gcovr pytest-cov -r test-requirements.txt | |||
pip install numpy | |||
pip install numpy setuptools |
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.
Needed here because we use setup.py devlop
directly, and therefore there is no automatic build dep installation.
python -m pip install --upgrade pytest setuptools | ||
python setup.py build_ext --inplace | ||
python -m pip install setuptools | ||
python setup.py develop |
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.
Initially I had this as python -m pip install --editable .
. It seems this is causing the issue seen with pytest.
I agree that some if the sdist building process being missing from CI is an issue. We could add a minirelease build test to the process. I often find and fix a half dizrn problems in the release process though most if them are just keeping up with azure changes or getting minor changes to the source tree working. I will send you the link to the location of the artifacts when I get to the office. |
To see the artifacts. Start at the jpype project page: https://dev.azure.com/jpype-project/jpype Click Pipelines => jpype.release https://dev.azure.com/jpype-project/jpype/_build/results?buildId=1319&view=results Chose a failed build. Scroll the the bottom of the page. Then choose the Initial. The artifacts used to build everything are stored there. The release artifacts are in the next stage, but as they all failed that is empty. In case I am ever run over by a bus, the whole procedure for creating releases is under doc. Assuming everything is in order, simply trigger the release pipeline on releases/vX.X.X branch and then copy all the wheels back to github release with the change log. |
Any progress on this? |
Unfortunately not due to a bunch of other things coming up over the last week. My two avenues to explore are whether we have a bad clone of the repository (and therefore setuptools-scm isn't registering This PR itself is fine to roll in my opinion though |
Okay I will try to merge it into the release. I ran the release process locally and the problem is that I don't see a problem. The jar was built so it can't be your PR. The wheels build completely so it isn't that. I am going to try fresh on dev and see if we just hit a random fluke issue. I am stumped thus far because unless I can see a difference between the local and dev builds I am going to have to hunt and peck running the release over and over with tiny changes. I guess I can start by just running the release pipeline on the last release and see if perhaps it was a change on azure rather than something in our code. |
I believe I found the source of at least one error. The problem is that the packaging system has changed the case of the source distribution from "JPype1" to "jpype1" thus causing everything downstream to fail to pick up the package. I didn't see anything in the pyproject that changes the case so it must be something in the Python dist tool itself. |
Apparently, poetry decided that you can't use package names because of a style guide. This pretty much breaks our reelase scripts because while pip isn't case sensitive, the build systems certainly are. |
I pulled the wheels and I see nothing wrong with them. It appears to be something to do with the ability to test. Either the wheel does not install properly or our test procedure is busted. |
I have identified the second issue. The problem is the python search path. We want to test the wheel we have built which contains the jar file, but Python always defaults to the local directory first. As the local directory was never built there is no org.jpype.jar to be found. Hence a perfectly working wheel will look like it is failing. I am not sure why this never happened in the past. It is of course related to the pyproject change as the old setup built in place. We are supposed to be checking the installed wheel not the state of pulled files (after all it may have stuff we didn't ship.) SO technically it is better to not build in place. There doesn't seem to be an easy way to make python ignore local directories from the command line. I will try removing the jpype directory, moving it, or running from another directory as an option. |
Yes, agreed that it is good that we fix this properly - it can detect real packaging issues.
This is the usual approach I've seen (and implemented).
Could be that somewhere in the pipeline definition we changed from
I think isolated mode might be handy there... the |
I'm trying to reproduce the issue discussed in #1232 (comment).