这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@Maegereg
Copy link
Contributor

@Maegereg Maegereg commented Oct 1, 2025

Part of #29

This adds the tag check and build jobs, including mac builds for intel and arm.

I did have to make one tweak to the build process. We'd previously set the MACOS_DEPLOYMENT_TARGET environment variable (which ensures that we're building wheels that are MacOS 11+ compatible, rather than 15+ based on VM OS) in the cibuildwheel settings. But, when we do uv run nox -s build on a fresh machine, we first build as part of the uv run call (which installs the current project), and cibuildwheel (which gets called in the nox session) re-uses the binaries built in that initial step. In other words, the caching means that we need the environment variable set whenever we're building the libraries, not just when we're using cibuildwheel. I left the variable set in the cibuildwheel configuration so that we're specifying the target MacOS version explicitly, rather than relying on the cibuildwheel defaults.

@Maegereg Maegereg requested a review from tmager October 1, 2025 23:31
@Maegereg Maegereg self-assigned this Oct 1, 2025
@tmager
Copy link
Contributor

tmager commented Oct 3, 2025

when we do uv run nox -s build on a fresh machine, we first build as part of the uv run call (which installs the current project), and cibuildwheel (which gets called in the nox session) re-uses the binaries built in that initial step

Do we need to build in the uv run call? Doing the build only within cibuildwheel seems like a more obvious behavior to me, but I could be convinced otherwise. IIRC this at one point worked by just failing the build if cibuildwheel found existing binaries, perhaps we should go back to that approach?

@Maegereg
Copy link
Contributor Author

Maegereg commented Oct 3, 2025

Do we need to build in the uv run call?

Let me see if I can figure out how to avoid it. uv run installs the project by default, and I didn't see a handy --no-install-project flag for it the way there is with uv sync, but there's almost certainly a way to do this.

IIRC this at one point worked by just failing the build if cibuildwheel found existing binaries, perhaps we should go back to that approach?

That means nox -s build will fail on pretty much any developer machine, which seems undesirable. But maybe I can make that check only happen in the CI? Or, another option is the same genre would be to delete any build libraries first.

@Maegereg Maegereg force-pushed the dasm/release-build branch 4 times, most recently from 5b3e8d5 to d2238ff Compare October 3, 2025 23:53
@Maegereg
Copy link
Contributor Author

Maegereg commented Oct 3, 2025

Okay, I think this is much better now. I have avoided building Core as part of uv run, and I have added a cibuildwheel check that will fail if the libraries have been built (but only on a CI runner). I also removed the part that added the MACOS_DEPLOYMENT_TARGET to every build session, since we probably don't care if we're building dev wheels, and it's one less thing to keep up to date.

@Maegereg Maegereg force-pushed the dasm/release-build branch from d2238ff to c39afec Compare October 3, 2025 23:56
Copy link
Contributor

@tmager tmager left a comment

Choose a reason for hiding this comment

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

Ok, this seems much clearer now, thanks for looking into it more. LGTM barring one minor comment.

@Maegereg Maegereg added this pull request to the merge queue Oct 4, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 4, 2025
@Maegereg Maegereg added this pull request to the merge queue Oct 6, 2025
Merged via the queue into main with commit d046ca1 Oct 6, 2025
3 checks passed
@Maegereg Maegereg deleted the dasm/release-build branch October 6, 2025 17:42
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.

3 participants