Skip to content

Conversation

@Smjert
Copy link
Member

@Smjert Smjert commented Oct 19, 2024

  • Change the installed version to 3.13.x
  • Don't restrict to a single version on macOS, but let newer minor versions be installed
  • Don't use x64 python on macOS arm runners
  • Fix python binary substitution in the test files when builder architecture
    doesn't correspond with tester architecture.
  • Use a sed separator that doesn't create problems with BSD sed, and isn't a regex operator
  • Correct the in place option usage if the package_tests.sh is ever used with BSD sed,
    since it doesn't work the same as GNU sed

@Smjert Smjert added the CI/CD Anything about the Continuous Integration or Continuous Deployment tool used by the repository label Oct 19, 2024
@Smjert Smjert closed this Oct 20, 2024
@Smjert Smjert reopened this Oct 20, 2024
@Smjert Smjert force-pushed the stefano/ci/update-python-version branch from 12d1df4 to 1f9f318 Compare October 20, 2024 14:33
@Smjert Smjert closed this Oct 20, 2024
@Smjert Smjert reopened this Oct 20, 2024
@michael-myers
Copy link
Contributor

  • Don't restrict to a specific version of python on macOS

I could be reading the change wrong but doesn't this change still restrict the version of Python?

  • Don't use x64 python on macOS arm runners

By removing the line that specifies architecture as x64, doesn't setup-python still default to x64?

@Smjert
Copy link
Member Author

Smjert commented Oct 21, 2024

I could be reading the change wrong but doesn't this change still restrict the version of Python?

This is mostly referring to the fact that Python for the macOS runner was specifically restricted to version 3.12.2, while in other cases it was 3.x. It's true that in general I changed the default to be 3.13.x, but the decision here is to get bugfixes but not versions that can break things, if we are not ready to handle them.
I'll clarify this in the message.

By removing the line that specifies architecture as x64, doesn't setup-python still default to x64?

If you check the CI it's actually defaulting to arm64. It's indeed a bit confusing because leaving the default for the macOS-12 runner, it choose the arm64 architecture which definitely was not correct (and in fact it failed).
Also it seemed by the documentation that you could only choose between x86 and x64, but checking in the code they seem to support the arm64 string, it's not just properly documented.

I'll add back the architecture but with arm64 to be explicit.

@michael-myers
Copy link
Contributor

michael-myers commented Oct 21, 2024

That's what I get for trusting documentation! :-P

Edit: I've opened an issue on their repo actions/setup-python#966

@Smjert Smjert force-pushed the stefano/ci/update-python-version branch 8 times, most recently from d074025 to 982a844 Compare October 24, 2024 23:45
- Change the installed version to 3.13.x
- Don't restrict to a single version on macOS, but let newer minor versions be installed
- Don't use x64 python on macOS arm runners
- Fix python binary substitution in the test files when builder architecture
  doesn't correspond with tester architecture.
- Use a sed separator that doesn't create problems with BSD sed, and isn't a regex operator
- Correct the in place option usage if the package_tests.sh is ever used with BSD sed,
  since it doesn't work the same as GNU sed
@Smjert Smjert force-pushed the stefano/ci/update-python-version branch from 982a844 to 51813fb Compare October 25, 2024 00:16
@Smjert Smjert marked this pull request as ready for review October 25, 2024 12:03
@Smjert Smjert requested review from a team as code owners October 25, 2024 12:03
@Smjert Smjert merged commit 6ad960c into osquery:master Oct 26, 2024
@Smjert Smjert deleted the stefano/ci/update-python-version branch October 26, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI/CD Anything about the Continuous Integration or Continuous Deployment tool used by the repository

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants