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.

Sign up for GitHub

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

Jump to bottom

Fix macOS BigSur #57859

Merged
merged 7 commits into from
Jul 14, 2020
Merged

Fix macOS BigSur #57859

merged 7 commits into from
Jul 14, 2020

Conversation

weswhet
Copy link
Contributor

@weswhet weswhet commented Jul 2, 2020 •
edited
Loading

What does this PR do?

Adds basic support for macOS Big Sur.

What issues does this PR fix or reference?

Fixes #57787

Previous Behavior

Salt would crash on Big Sur and get 😭. See the issue above for more details.

New Behavior

salt + Big Sur = ❤️

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

@weswhet weswhet requested a review from a team as a code owner July 2, 2020 22:56
@ghost ghost requested review from krionbsd and removed request for a team July 2, 2020 22:57
@weswhet
Copy link
Contributor Author

weswhet commented Jul 2, 2020 •
edited
Loading

@kaorihinata mind taking a look? I tried to clean it up into a single if darwin statement.

@sagetherage Can we get this into 3001.1 so more folks can test Big Sur?

@weswhet
Copy link
Contributor Author

weswhet commented Jul 2, 2020

Pairs well with #57607 but not required.

@weswhet
Copy link
Contributor Author

weswhet commented Jul 3, 2020

re-run all

@kaorihinata
Copy link
Contributor

kaorihinata commented Jul 3, 2020 •
edited
Loading

os_version(...) doesn't appear to be necessary. Is there a reason you'd like to add another function and tests when you only use 1 form of it?

Something like:

platform.mac_ver()[0].startswith('10.15')
(or '10.15.' if mac_ver() includes the 0.)

...or:

platform.mac_ver()[0].split('.')[:2] == ['10', '15']
(Python itself uses this in places now that I check.)

...or:

platform.mac_ver()[0][:6] in {'10.15', '10.15.'}
(Because slice behavior on an out of range end index is nifty like that.)

...would also suffice, aye? I don't see os_version(...) as enough of a departure from the functionality platform already provides for us to need another method of mangling the version.

@kaorihinata
Copy link
Contributor

kaorihinata commented Jul 4, 2020 •
edited
Loading

@weswhet Now that I've had time to go over the build logs, it seems like relying on salt.utils.mac_utils to get to os_version is running into issues when rolled into a thin tarball (possibly a circular import involving salt.modules.cmdmod.) I can reproduce the issue by building a thin tarball, creating a test script that modifies the module search path, then attempting to import salt.modules.cmdmod. Any ideas? Alternatively, you could probably just drop os_version and use a 1 liner.

Edit: Seems to have something to do with lacking the module loading magic that salt-call uses to work around all of the cross importing. salt.utils.mac_utils is probably just not the right place to put os_version if you want to include it.

@sagetherage
Copy link
Contributor

@weswhet

Can we get this into 3001.1 so more folks can test Big Sur?

we need to get it passing tests here, it may need some fixes to get merged to do that, I will review today.

@kaorihinata
Copy link
Contributor

Remove the dependency on salt.utils.mac_utils and you fix unit.utils.test_thin.SSHThinTestCase.test_thin_dir. This may also fix integration.ssh.test_deploy.SSHTest.test_set_path as a side effect.

@weswhet
Copy link
Contributor Author

weswhet commented Jul 6, 2020

Remove the dependency on salt.utils.mac_utils and you fix unit.utils.test_thin.SSHThinTestCase.test_thin_dir. This may also fix integration.ssh.test_deploy.SSHTest.test_set_path as a side effect.

Done, I added the mac_utils os piece because I have some other code I was going to PR but I've scratched it for this case. Thanks for the review/feedback. Appreciate it!

@kaorihinata
Copy link
Contributor

os_version is fine if it's going to be used more widely in the future, but we'll need to find somewhere to put it that doesn't require importer/loader magic to make it work. Also looks like we're already going to need to re-run for one of the Windows hosts.

@weswhet
Copy link
Contributor Author

weswhet commented Jul 6, 2020

re-run windows2016

@kaorihinata
Copy link
Contributor

re-run opensuse15

krionbsd
krionbsd previously approved these changes Jul 7, 2020
View reviewed changes
@krionbsd krionbsd added Merge Ready v3001.1 vulnerable version labels Jul 7, 2020
s0undt3ch
s0undt3ch reviewed Jul 7, 2020
View reviewed changes
salt/utils/rsax931.py Outdated Show resolved Hide resolved
kaorihinata
kaorihinata reviewed Jul 7, 2020
View reviewed changes
salt/utils/rsax931.py Outdated
@@ -30,6 +31,24 @@ def _find_libcrypto():
"""
if sys.platform.startswith("win"):
lib = str("libeay32")
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that @s0undt3ch mentions it, here as well.

Copy link
Contributor

@kaorihinata kaorihinata Jul 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ahh, found it. There was a comment here previously noting that Windows required the str call. Is this still the case, or was it an artifact of ancient (early Python 2, perhaps) history?

Edit: I know it wasn't your change (it was lost when I had to split the function for testing previously), but if you were copying this style below on line 51, then perhaps we should comment it again to avoid future confusion.

Copy link
Contributor Author

@weswhet weswhet Jul 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

added the comment back in for windows and removed the str cast for macOS as it's not required.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@twangboy do we still need this str() cast? I believe this was only necessary for Py2, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I validated that on windows salt with py3 the cast is not needed. @s0undt3ch how do you want to proceed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nuke it too :explosion: :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, Please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@s0undt3ch changed and ready to go :)

@weswhet
Copy link
Contributor Author

weswhet commented Jul 7, 2020

@krionbsd ready for re-review. I made changes based on recommendations from @s0undt3ch.

@s0undt3ch
Copy link
Collaborator

It's properly labeled and when tests pass, queued for the 3001.1 release.

@dwoz
Copy link
Contributor

dwoz commented Jul 13, 2020

re-run full mac

@dwoz dwoz merged commit 74e7d46 into saltstack:master Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaorihinata kaorihinata kaorihinata left review comments

@s0undt3ch s0undt3ch s0undt3ch left review comments

@krionbsd krionbsd krionbsd left review comments

Assignees
No one assigned
Labels
v3001.1 vulnerable version
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG][MacOS] Salt errors on macOS Big Sur
6 participants
@weswhet @kaorihinata @sagetherage @s0undt3ch @dwoz @krionbsd