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
System sass #219
System sass #219
Conversation
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'm still hesitant on taking something like this:
- It complicates our (already significantly complicated!) setup.py
- It is not a pathway we want to support as version mismatches cause lots of problems (and
libsass
itself is quite the moving target).
setup.py
Outdated
@@ -141,14 +147,26 @@ def restore_cencode(): | |||
else: | |||
link_flags = ['-fPIC', '-lstdc++'] | |||
|
|||
if system_sass: |
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.
instead of three if system_sass:
blocks, can you combine these?
setup.py
Outdated
@@ -14,55 +14,60 @@ | |||
|
|||
from setuptools import Extension, setup | |||
|
|||
LIBSASS_SOURCE_DIR = os.path.join('libsass', 'src') | |||
system_sass = os.environ.get('SYSTEMSASS', False) |
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.
this looks like SYSTEMS ASS to me :), how about SYSTEM_SASS
?
This looks fine to me, curious to see what @dahlia says
It looks good to me. It's okay to merge if the builds succeed. |
setup.py
Outdated
@@ -149,9 +149,9 @@ def customize_compiler(compiler): | |||
flags.append('-mmacosx-version-min=10.7',) | |||
if tuple( | |||
map(int, platform.mac_ver()[0].split('.')) | |||
) >= (10, 9): | |||
) >= (10, 9): |
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 think the correct response to this lint is to indent the map
line by one more space instead of the paren
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.
Or save width a simpler and cleaner way:
macver = tuple(map(int, platform.mac_ver()[0].split('.')))
if macver >= (10, 9):
setup.py
Outdated
@@ -148,8 +148,8 @@ def customize_compiler(compiler): | |||
if platform.system() == 'Darwin': | |||
flags.append('-mmacosx-version-min=10.7',) | |||
if tuple( | |||
map(int, platform.mac_ver()[0].split('.')) | |||
) >= (10, 9): | |||
map(int, platform.mac_ver()[0].split('.')) |
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.
this should be indented 8 more spaces
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.
Why is that?
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.
https://www.python.org/dev/peps/pep-0008/#indentation
notably the part where it says:
"More indentation included to distinguish this from the rest."
Thanks! |
This has been released as part of 0.13.3! |
This PR implements solution for using system-installed libsass library to build the python sass extension.
This will help to get rid of some hacks/downstream-patches in Fedora and other distributions' packaging.