-
Notifications
You must be signed in to change notification settings - Fork 72
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
pin pymssql to feature branch to work around m1 build issues #3829
pin pymssql to feature branch to work around m1 build issues #3829
Conversation
@adamsachs I'll take a look here! |
@adamsachs Ok here is a quick summary of what's going on here The reason that it is currently breaking is that So what's happening here is that because we're pinning a specific git commit, the local machine is needing to fully rebuild (there is no wheel for that commit) the package, which expects all of those database dependencies to be available. Generally, they aren't, which leads to these failures. I was able to get it to fail locally using a Investigative steps:
The next thing I need confirmation on is if it works for ARM, I'm guessing not since you had already tried the |
Passing run #3249 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #3829 +/- ##
==========================================
+ Coverage 85.39% 87.18% +1.79%
==========================================
Files 317 317
Lines 19350 19350
Branches 2485 2485
==========================================
+ Hits 16523 16870 +347
+ Misses 2384 2049 -335
+ Partials 443 431 -12 ☔ View full report in Codecov by Sentry. |
ahhh thanks a bunch for explaining @ThomasLaPiana! this was a missing piece that explains some of my confusion around investigative steps the other day. i am indeed still getting errors building locally on the latest you've pushed to the branch, i'll post the stack trace below. i can also start playing around with this a bit more today though, your added insight may help unblock me a bit. also happy to pair up if that'll be easier!
|
OK - after banging our heads on desks for longer than is healthy, we seem to have found the "sweet spot" - though i don't think you can call it that. i'll try to recap how we arrived where we did so that we can track why we did what we did, and hopefully can remove this hacky workaround if/when things improve in the upstream libs moving forward 🤞.
that's how we've ended up at this point -- and as a consequence, different versions of excerpt of stack trace when trying to
|
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.
I made sure this worked "live", but it wouldn't hurt to have others verify as well!
@ThomasLaPiana i also made sure my server started locally and worked with basic smoke tests. i've also added the unsafe CI checks label and re-triggered CI so we can hopefully get the external tests running and get a bit more test coverage. did you have something else in mind before we merge? |
I cannot, ship it! |
Co-authored-by: Thomas <[email protected]> Co-authored-by: Thomas <[email protected]>
noting for posterity - i never got external tests running on the CI for this PR itself (silly quirks with our this helps to solidify that this change doesn't give us significant integration regressions wrt mssql |
Closes #3824
Description Of Changes
see comment recapping where we landed and why 😄
Code Changes
pymssql
to a version range that can work both for docker builds and local pip installs on an M1cython
version and thenpymssql
on aDockerfile
build steps;pymssql
build step also includes theno-build-isolation
flagpymssql
functionality!Steps to Confirm
Pre-Merge Checklist
CHANGELOG.md