-
Notifications
You must be signed in to change notification settings - Fork 1
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
{2023.06}[foss/2022a] Pillow V9.1.1 #202
{2023.06}[foss/2022a] Pillow V9.1.1 #202
Conversation
Instance
|
Instance
|
Instance
|
Instance
|
bot: build inst:AWS repo:nessi-2023.06-swl-deb11 arch:x86_64/generic |
Updates by the bot instance
|
Updates by the bot instance
|
Updates by the bot instance
|
Updates by the bot instance
|
New job on instance
|
New job on instance
|
New job on instance
|
New job on instance
|
New job on instance
|
New job on instance
|
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.
Looks good overall. Some suggestions to keep the overall design of the hooks file consistent.
Instead of setting the envvars, the EESSI_* paths should be added. Otherwise, existing values could be overwritten.
eb_hooks.py
Outdated
@@ -33,6 +33,18 @@ def get_eessi_envvar(eessi_envvar): | |||
|
|||
return eessi_envvar_value | |||
|
|||
def set_Pillow_envvars(ec): |
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.
Should be named similarly to the other parse_hook
s and the arguments should have eprefix
too, for example,
def parse_hook_pillow_set_cpath_library_path(ec, eprefix):
The whole function should be moved to where it fits alphabetically (alphabetically ordered functions help in finding a function).
eb_hooks.py
Outdated
EESSI_CPATH = os.getenv('EESSI_EPREFIX') + '/usr/include' | ||
EESSI_LIB_PATH = os.getenv('EESSI_EPREFIX') + '/usr/lib64' |
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.
These two lines should probably be moved inside the if
block?
eb_hooks.py
Outdated
os.environ['CPATH'] = os.pathsep + EESSI_CPATH | ||
os.environ['LIBRARY_PATH'] = os.pathsep + EESSI_LIB_PATH |
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.
These lines set the environment variables. The description of the PR says adding. I think it would be better to keep existing values, maybe something like the following could work
os.environ['CPATH'] = os.pathsep.join(os.environ.get('CPATH', ''), EESSI_CPATH)
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.
If this works, could be checked in an interactive Python session.
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 am setting the CPATH, but adding to the existing CPATH value, yet we can set whichever suits best.
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.
The following could work
os.environ['CPATH'] = os.pathsep.join(filter(None,[os.environ.get('CPATH',''), EESSI_CPATH]))
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 am setting the CPATH, but adding to the existing CPATH value, yet we can set whichever suits best.
CPATH
may already be set to some value. If so it would be overwritten. That could lead then to other issues difficult to debug.
eb_hooks.py
Outdated
print_msg("NOTE: For Pillow which has Szip as a dependancy, CPATH has been set to %s", os.getenv('CPATH')) | ||
print_msg("NOTE: For Pillow which has Szip as a dependancy, LIBRARY_PATH has been set to %s", os.getenv('LIBRARY_PATH')) | ||
ec.log.info("NOTE: For Pillow which has Szip as a dependancy, CPATH has been set to %s", os.getenv('CPATH')) | ||
ec.log.info("NOTE: For Pillow which has Szip as a dependancy, LIBRARY_PATH has been set to %s", os.getenv('LIBRARY_PATH')) |
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.
Here Szip
is mentioned while earlier not finding zlib
was the problem.
The messages could be a bit improved (particularly when adding directories), by stating something like
original CPATH (OLD_VALUE) has been extended with EESSI_CPATH, new CPATH is NEW_VALUE
or just
CPATH (original value OLD_VALUE) has been extended with EESSI_CPATH
eb_hooks.py
Outdated
@@ -62,6 +74,7 @@ def parse_hook(ec, *args, **kwargs): | |||
# determine path to Prefix installation in compat layer via $EPREFIX | |||
eprefix = get_eessi_envvar('EPREFIX') | |||
|
|||
set_Pillow_envvars(ec) |
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.
The new hook should be called via line (just 2 lines below where it is called now)
PARSE_HOOKS[ec.name](ec, eprefix)
This requires that a line
'Pillow': parse_hook_pillow_set_cpath_library_path,
is added to the PARSE_HOOKS
dictionary (near the end of the file).
eessi-2023.06-eb-4.8.1-2022a.yml
Outdated
@@ -51,3 +51,7 @@ easyconfigs: | |||
- Tk-8.6.12-GCCcore-11.3.0.eb | |||
- GROMACS-2023.1-foss-2022a.eb | |||
- MUMPS-5.5.1-foss-2022a-metis.eb | |||
- Pillow-9.1.1-GCCcore-11.3.0.eb: | |||
# Uses a custom hook since has Szip as dependency which has hard coded header and library path within Pillow code. |
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.
Is it Szip
or zlib
?
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.
100%, will fix
2d45617
to
9c5daa9
Compare
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.
Few additional suggestions.
eb_hooks.py
Outdated
@@ -66,6 +66,23 @@ def parse_hook(ec, *args, **kwargs): | |||
PARSE_HOOKS[ec.name](ec, eprefix) | |||
|
|||
|
|||
def parse_hook_pillow_set_cpath_library_path(ec, eprefix): | |||
"""Get EESSI_CPATH environment variable from the environment""" |
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.
Actually, the hook augments env vars CPATH
and LIBRARY_PATH
with paths including the EESSI_PREFIX.
Mention LIBRARY_PATH
too?
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.
Does this look good : Extend CPATH and LIBRARY_PATH environment variables using EESSI_EPREFIX.
eb_hooks.py
Outdated
print_msg("NOTE:Pillow has zlib as a dependancy,the original CPATH value: (%s) has been extended with (%s)", | ||
os.getenv('CPATH'), EESSI_CPATH) | ||
print_msg("NOTE:Pillow has zlib as a dependancy,the original LIBRARY_PATH value: (%s) has been extended with (%s)", | ||
os.getenv('LIBRARY_PATH'), EESSI_LIB_PATH) | ||
ec.log.info("NOTE:Pillow has zlib as a dependancy,the original CPATH value: (%s) has been extended with (%s)", | ||
os.getenv('CPATH'), EESSI_CPATH) | ||
ec.log.info("NOTE:Pillow has zlib as a dependancy,the original LIBRARY_VALUE value: (%s) has been extended with (%s)", | ||
os.getenv('LIBRARY_PATH'), EESSI_LIB_PATH) |
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.
Minor suggestion on formatting. Added spaces.
"NOTE: Pillow has zlib as a dependency. The original CPATH ..."
@@ -273,6 +290,7 @@ def pre_test_hook_ignore_failing_tests_SciPybundle(self, *args, **kwargs): | |||
'fontconfig': parse_hook_fontconfig_add_fonts, | |||
'OpenBLAS': parse_hook_openblas_relax_lapack_tests_num_errors, | |||
'UCX': parse_hook_ucx_eprefix, | |||
'Pillow': parse_hook_pillow_set_cpath_library_path, |
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.
Could you order the entries alphabetically ... one line upwards.
eb_hooks.py
Outdated
@@ -66,6 +66,23 @@ def parse_hook(ec, *args, **kwargs): | |||
PARSE_HOOKS[ec.name](ec, eprefix) | |||
|
|||
|
|||
def parse_hook_pillow_set_cpath_library_path(ec, eprefix): |
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.
Could you move the whole function to where the other parse hooks are defined?
Checklist for building Pillow V9.1.1:
|
Target architectures
Checklist for deployment/ingestion
command & logcommand
-rw-r--r--. 1 cvmfs cvmfs 1600 Oct 10 12:43 aarch64/generic/modules/all/Pillow/9.1.1-GCCcore-11.3.0.lua aarch64/generic/software/Pillow/9.1.1-GCCcore-11.3.0: x86_64/amd/zen2/software/Pillow/9.1.1-GCCcore-11.3.0: x86_64/generic/software/Pillow/9.1.1-GCCcore-11.3.0: x86_64/intel/broadwell/software/Pillow/9.1.1-GCCcore-11.3.0: x86_64/intel/cascadelake/software/Pillow/9.1.1-GCCcore-11.3.0: x86_64/intel/skylake_avx512/software/Pillow/9.1.1-GCCcore-11.3.0:
|
The actual build issue with Pillow is that it can't find zlib through the hardcoded paths, the problem is solved by using --disable-platform-guessing (which was introduced in easybuilders/easybuild-easyconfigs#18881), and adding the appropriate paths to $CPATH and $LIBRARY_PATH via a hook.
License: https://spdx.org/licenses/HPND.html
Will install the following package: