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.

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

salt 3000.1 #52835

Closed
wants to merge 1 commit into from
Closed

Conversation

EricFromCanada
Copy link
Member

Created with brew bump-formula-pr.

@EricFromCanada
Copy link
Member Author

Now refuses to run unless a huge list of pyobjc dependencies are installed first.

@Bo98
Copy link
Member

Bo98 commented Apr 11, 2020

I don't even see what they use pyobjc for...

@bayandin
Copy link
Member

bayandin commented Apr 21, 2020

Can we drop this list of resources and install them as salt dependencies (as we do for awscli for example)?

@SMillerDev
Copy link
Member

 ==> brew linkage --test salt
Missing libraries:
  /System/Library/Frameworks/Network.framework/Versions/A/Network
  /System/Library/Frameworks/UserNotifications.framework/Versions/A/UserNotifications

@Bo98
Copy link
Member

Bo98 commented Apr 21, 2020

Oh boy, it's PyObjC time.

@Bo98
Copy link
Member

Bo98 commented Apr 21, 2020

Can we drop this list of resources and install them as salt dependencies

Probably. Ideally we wouldn't unless upstream lock their versions, but there becomes a point where it is infeasible to vendor everything. We give other projects some slack, like everything that depends on ghc, so I think we can here too.

@bayandin
Copy link
Member

 ==> brew linkage --test salt
Missing libraries:
  /System/Library/Frameworks/Network.framework/Versions/A/Network
  /System/Library/Frameworks/UserNotifications.framework/Versions/A/UserNotifications

Both /System/Library/Frameworks/Network.framework/Versions/A/Network and /System/Library/Frameworks/UserNotifications.framework/Versions/A/UserNotifications requires macOS >= 10.14.

Not sure what is the right way to handle this. Just drop High Sierra support?

@Bo98
Copy link
Member

Bo98 commented Apr 21, 2020

Would it be bad if we just do (buildpath/"pkg/osx/req_pyobjc.txt").write ""?

@bayandin bayandin force-pushed the salt-3000.1 branch 2 times, most recently from c050aa3 to ed8b340 Compare April 21, 2020 23:53
@bayandin
Copy link
Member

bayandin commented Apr 22, 2020

pyobjc dependency was added in saltstack/salt#49657 to make it available for salt user scripts (andsalt doesn't use it itself).

I don't really like an idea of pathing packages in such way (on the contrary with what salt developers do). But on the other hand, it will just limit some feature on High Sierra, but other available.

Btw, the funniest thing is that salt can't be installed on Mac via pip right now (saltstack/salt#56108):

$ pip3 install -U salt==3000.1
  ...
  Running setup.py install for timelib ... error
    ERROR: Command errored out with exit status 1:
     command: /Users/bayandin/Desktop/venv/bin/python3.7 -u -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/private/var/folders/v5/ly_6fqkn1_l7g5w7lmwm_qlh0000gn/T/pip-install-oceegpay/timelib/setup.py'"'"'; __file__='"'"'/private/var/folders/v5/ly_6fqkn1_l7g5w7lmwm_qlh0000gn/T/pip-install-oceegpay/timelib/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' install --record /private/var/folders/v5/ly_6fqkn1_l7g5w7lmwm_qlh0000gn/T/pip-record-0v8rkfjk/install-record.txt --single-version-externally-managed --compile --install-headers /Users/bayandin/Desktop/venv/include/site/python3.7/timelib
         cwd: /private/var/folders/v5/ly_6fqkn1_l7g5w7lmwm_qlh0000gn/T/pip-install-oceegpay/timelib/
    Complete output (15 lines):
    running install
    running build
    running build_ext
    building 'timelib' extension
    creating build
    creating build/temp.macosx-10.15-x86_64-3.7
    creating build/temp.macosx-10.15-x86_64-3.7/ext-date-lib
    clang -Wno-unused-result -Wsign-compare -Wunreachable-code -fno-common -dynamic -DNDEBUG -g -fwrapv -O3 -Wall -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk -I/Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/usr/include -I/Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/System/Library/Frameworks/Tk.framework/Versions/8.5/Headers -I/usr/local/opt/jpeg-turbo/include -DHAVE_STRING_H=1 -I/usr/local/include -I/usr/local/opt/[email protected]/include -I/usr/local/opt/sqlite/include -I/Users/bayandin/Desktop/venv/include -I/usr/local/Cellar/python/3.7.7/Frameworks/Python.framework/Versions/3.7/include/python3.7m -c ext-date-lib/astro.c -o build/temp.macosx-10.15-x86_64-3.7/ext-date-lib/astro.o
    In file included from ext-date-lib/astro.c:26:
    In file included from ext-date-lib/timelib.h:24:
    ext-date-lib/timelib_structs.h:24:10: fatal error: 'timelib_config.h' file not found
    #include "timelib_config.h"
             ^~~~~~~~~~~~~~~~~~
    1 error generated.
    error: command 'clang' failed with exit status 1
    ----------------------------------------
ERROR: Command errored out with exit status 1: /Users/bayandin/Desktop/venv/bin/python3.7 -u -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/private/var/folders/v5/ly_6fqkn1_l7g5w7lmwm_qlh0000gn/T/pip-install-oceegpay/timelib/setup.py'"'"'; __file__='"'"'/private/var/folders/v5/ly_6fqkn1_l7g5w7lmwm_qlh0000gn/T/pip-install-oceegpay/timelib/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' install --record /private/var/folders/v5/ly_6fqkn1_l7g5w7lmwm_qlh0000gn/T/pip-record-0v8rkfjk/install-record.txt --single-version-externally-managed --compile --install-headers /Users/bayandin/Desktop/venv/include/site/python3.7/timelib Check the logs for full command output.

@bayandin bayandin self-assigned this Apr 22, 2020
@Bo98
Copy link
Member

Bo98 commented Apr 22, 2020

I suppose it might make sense in the context of a virtualenv, depending on whether said user scripts are able to play nicely with such setup.

PyObjC is almost always doomed to cause broken linkage on anything that's not the latest OS. It's because it's not lightweight - the PyObjC dependency pulls in modules for every supported system framework, which can mean new frameworks only present on recent OS versions. The reason it doesn't on Mojave is because Salt use an older version that predates Catalina.

@bayandin
Copy link
Member

PyObjC is almost always doomed to cause broken linkage on anything that's not the latest OS. It's because it's not lightweight - the PyObjC dependency pulls in modules for every supported system framework, which can mean new frameworks only present on recent OS versions. The reason it doesn't on Mojave is because Salt use an older version that predates Catalina.

Fair enough, let's try without PyObjC

Co-Authored-By: Alexander Bayandin <[email protected]>
@EricFromCanada EricFromCanada added the ready to merge PR can be merged once CI is green label Apr 22, 2020
@BrewTestBot
Copy link
Member

@bayandin has triggered a merge.

@EricFromCanada EricFromCanada deleted the salt-3000.1 branch April 22, 2020 19:29
@bayandin bayandin removed their assignment Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants