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

add libjpeg-turbo as dependency to recent LibTIFF easyconfigs, as otherwise the build can pick up the system-wide library #9146

Merged
merged 7 commits into from Nov 15, 2019

Conversation

paulmelis
Copy link
Contributor

(created using eb --new-pr)

@akesandgren
Copy link
Contributor

Minor comment.
It would be nice if you had the name of the software package in the title. Something like
LibTIFF: Add libjpeg-turbo as dependency,...
Or even LibTIFF-4.0.9: ...

Makes it much easier to figure out what it is about.

@paulmelis
Copy link
Contributor Author

Minor comment.
It would be nice if you had the name of the software package in the title. Something like
LibTIFF: Add libjpeg-turbo as dependency,...
Or even LibTIFF-4.0.9: ...

Makes it much easier to figure out what it is about.

Hmmm, okay, did not realize that it would need to be more specific. I was also half expecting the --new-pr command to take care of that, as you give it an easyconfig file from which it would be able to extract such metadata. Ah well, something to remember for next time.

@migueldiascosta
Copy link
Member

@paulmelis --new-pr sets the title automatically if no commit message is set, but since you are modifying an existing message a custom commit message is necessary (although --new-pr could probably be enhanced to add some context also in that case as you mention)

in any case, you can still edit the title at any time

@paulmelis
Copy link
Contributor Author

@paulmelis --new-pr sets the title automatically if no commit message is set, but since you are modifying an existing message a custom commit message is necessary (although --new-pr could probably be enhanced to add some context also in that case as you mention)

Wait, how am I "modifying an existing message"?

@paulmelis paulmelis changed the title Add libjpeg-turbo as dependency, as otherwise the build can pick up the system-wide library [LibTIFF-4.0.9-GCCcore-7.3.0.eb] Add libjpeg-turbo as dependency, as otherwise the build can pick up the system-wide library Oct 17, 2019
@migueldiascosta
Copy link
Member

sorry, I meant "modifying an existing easyconfig" :)

@migueldiascosta
Copy link
Member

Test report by @migueldiascosta
SUCCESS
Build succeeded for 1 out of 1 (1 easyconfigs in this PR)
generoso - Linux centos linux 7.6.1810, Intel(R) Xeon(R) CPU E5-2660 v4 @ 2.00GHz, Python 3.6.8
See https://gist.github.com/bbf6345ed217caab04170fab0e025973 for a full test report.

Copy link
Member

@migueldiascosta migueldiascosta left a comment

Choose a reason for hiding this comment

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

lgtm

@migueldiascosta
Copy link
Member

@paulmelis this was indeed a problem and your PR fixes it, thanks again

would you consider also fixing the other LibTIFF easyconfigs?

@paulmelis
Copy link
Contributor Author

Travis test report: 5/5 runs failed - see https://travis-ci.org/easybuilders/easybuild-easyconfigs/builds/599019907

Only showing partial log for 1st failed test suite run 17707.1;
full log at https://travis-ci.org/easybuilders/easybuild-easyconfigs/jobs/599019908

...
FAIL: Specific checks only done for the (easyconfig) files that were changed in a pull request.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/easybuilders/easybuild-easyconfigs/test/easyconfigs/easyconfigs.py", line 541, in test_changed_files_pull_request
    self.check_https(changed_ecs)
  File "/home/travis/build/easybuilders/easybuild-easyconfigs/test/easyconfigs/easyconfigs.py", line 485, in check_https
    self.assertFalse(failing_checks, '\n'.join(failing_checks))
AssertionError: Found http:// URL in LibTIFF-4.0.9-GCCcore-7.3.0.eb, should be https:// : http://hpcbios.readthedocs.org/en/latest/

You have got to be kidding me? That URL is part of a commented line:

So even though the description of check_https() says it checks URLs used for "homepage/source_urls" it does something different by simply matches any part of the easyconfig text without looking at lines and comments? Seriously?

def check_https(self, changed_ecs):
"""Make sure https:// URL is used (if it exists) for homepage/source_urls (rather than http://)."""
whitelist = [
'UCX-', # bad certificate for https://www.openucx.org
]
http_regex = re.compile('http://[^"\'\n]+', re.M)
failing_checks = []
for ec in changed_ecs:
ec_fn = os.path.basename(ec.path)
if any(ec_fn.startswith(x) for x in whitelist):
continue
for http_url in http_regex.findall(ec.rawtxt):
https_url = http_url.replace('http://', 'https://')
try:
https_url_works = bool(urlopen(https_url))
except (HTTPError, URLError):
https_url_works = False
if https_url_works:
failing_checks.append("Found http:// URL in %s, should be https:// : %s" % (ec_fn, http_url))
self.assertFalse(failing_checks, '\n'.join(failing_checks))

@migueldiascosta
Copy link
Member

@paulmelis it does seem a bit too much... would you mind opening an issue about this?

@paulmelis
Copy link
Contributor Author

@paulmelis it does seem a bit too much... would you mind opening an issue about this?

Done, #9180

@migueldiascosta
Copy link
Member

closing and reopening to trigger travis

@easybuilders easybuilders deleted a comment from boegelbot Nov 6, 2019
@easybuilders easybuilders deleted a comment from boegelbot Nov 6, 2019
@boegel
Copy link
Member

boegel commented Nov 6, 2019

Thanks a lot for the fix @paulmelis!

We should apply this consistently though, at least to all LibTIFF 4.0.9 & newer easyconfigs (which may involve adding the missing easyconfigs for libjpeg-turbo with other toolchains)...

@boegel
Copy link
Member

boegel commented Nov 6, 2019

I looked into making this consistent myself, no missing libjpeg-turbo easyconfigs.

@paulmelis please review/merge sara-nl#3 to update this PR

consistently add missing libjpeg-turbo dependency in LibTIFF 4.0.9 and 4.0.10 easyconfigs
@boegel
Copy link
Member

boegel commented Nov 6, 2019

Test report by @boegel
SUCCESS
Build succeeded for 8 out of 8 (8 easyconfigs in this PR)
node2409.golett.os - Linux centos linux 7.6.1810, Intel(R) Xeon(R) CPU E5-2680 v3 @ 2.50GHz, Python 2.7.5
See https://gist.github.com/782d1981013c4fa0407b3bd851aa3804 for a full test report.

stick to libjpeg-turbo 1.5.3 as dep for LibTIFF 4.0.9 w/ GCCcore/6.4.0
…adding libjpeg-turbo dep, it's beyond fixing now due to conflicts in 2017b vs 2018a easyconfigs...
@easybuilders easybuilders deleted a comment from boegelbot Nov 15, 2019
@easybuilders easybuilders deleted a comment from boegelbot Nov 15, 2019
@boegel boegel changed the title [LibTIFF-4.0.9-GCCcore-7.3.0.eb] Add libjpeg-turbo as dependency, as otherwise the build can pick up the system-wide library add libjpeg-turbo as dependency to recent LibTIFF easyconfigs, as otherwise the build can pick up the system-wide library Nov 15, 2019
@boegel
Copy link
Member

boegel commented Nov 15, 2019

Test report by @boegel
SUCCESS
Build succeeded for 8 out of 8 (8 easyconfigs in this PR)
node2414.golett.os - Linux centos linux 7.7.1908, Intel(R) Xeon(R) CPU E5-2680 v3 @ 2.50GHz, Python 2.7.5
See https://gist.github.com/f1ddef7056d14838839b186cb61e528d for a full test report.

@boegel
Copy link
Member

boegel commented Nov 15, 2019

Going in, thanks @paulmelis!

@boegel boegel merged commit 1e63f1a into easybuilders:develop Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants