Skip to content

{tools}[GCCcore/6.4.0] ncompress v4.2.4.4#4852

Merged
boegel merged 3 commits intoeasybuilders:developfrom
JackPerdue:20170711083646_new_pr_ncompress4244
Jul 13, 2017
Merged

{tools}[GCCcore/6.4.0] ncompress v4.2.4.4#4852
boegel merged 3 commits intoeasybuilders:developfrom
JackPerdue:20170711083646_new_pr_ncompress4244

Conversation

@JackPerdue
Copy link
Copy Markdown
Contributor

(created using eb --new-pr)

@vanzod vanzod added this to the 3.3.1 milestone Jul 11, 2017
vanzod
vanzod previously requested changes Jul 11, 2017

toolchain = {'name': 'GCCcore', 'version': '6.4.0'}

# https://github.com/vapier/ncompress/archive/v4.2.4.4.tar.gz
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please remove

# https://github.com/vapier/ncompress/archive/v4.2.4.4.tar.gz
source_urls = ['https://github.com/vapier/ncompress/archive/']
sources = ['%(name)s-%(version)s.tar.gz']

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add SHA256 checksum

DESTDIR=%(installdir)s\
BINDIR=/bin\
MANDIR=/man/man1\
"""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add one line at a time to installopts with the += operator.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Or just collapse to a single line, I see no reason to spread this across multiple lines? In fact, the trailing \ characters are confusing imho...

installopts = "DESTDIR=%(installdir)s BINDIR=/bin MANDIR=/man/man1"

@boegel boegel modified the milestones: 3.3.1, 3.4.0 Jul 12, 2017
toolchain = {'name': 'GCCcore', 'version': '6.4.0'}

source_urls = ['https://github.com/vapier/ncompress/archive/']
sources = ['%(name)s-%(version)s.tar.gz']
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

download fails for me, should be v%(version)s.tar.gz?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wish there was a GITHUB_SOURCE template. :) Github is always confusing. I think there is a way (sometimes) to retrieve using the full SOURCE_TAR_GZ. In my browser if I click the v4.2.4.4.tar.gz it somehow manages to save it using the SOURCE_TAR_GZ name (and not just the v%(version).tar.gz name.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, it does a sort of rename, maybe if you use the releases URL rather than archive?

We actually have a template for GitHub URLs, i.e. GITHUB_SOURCE see easybuilders/easybuild-framework#1872.

It hasn't been used much though, and it has the caveat of also needing to define github_account = '...' in the easyconfig file...

@easybuilders easybuilders deleted a comment from boegelbot Jul 13, 2017
DESTDIR=%(installdir)s\
BINDIR=/bin\
MANDIR=/man/man1\
"""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Or just collapse to a single line, I see no reason to spread this across multiple lines? In fact, the trailing \ characters are confusing imho...

installopts = "DESTDIR=%(installdir)s BINDIR=/bin MANDIR=/man/man1"

@JackPerdue
Copy link
Copy Markdown
Contributor Author

Y'all are gonna luv my Mesa/17.1.2.... it has configopts that go on and on and..

I think this is clearer even if the backslashes take some getting used to.

@boegel
Copy link
Copy Markdown
Member

boegel commented Jul 13, 2017

@JackPerdue Consistent style across easyconfigs beats personal preferences imho. We're not using this multi-line style anywhere right now...

@boegel
Copy link
Copy Markdown
Member

boegel commented Jul 13, 2017

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

@boegel boegel dismissed vanzod’s stale review July 13, 2017 17:03

remarks fixed

@boegel
Copy link
Copy Markdown
Member

boegel commented Jul 13, 2017

Going in, thanks @JackPerdue!

@boegel
Copy link
Copy Markdown
Member

boegel commented Jul 13, 2017

Test report by @boegel
SUCCESS
Build succeeded for 1 out of 1 (1 easyconfigs in this PR)
node2035.delcatty.os - Linux centos linux 7.3.1611, Intel(R) Xeon(R) CPU E5-2670 0 @ 2.60GHz, Python 2.7.5
See https://gist.github.com/5fea45bc80eaf4c1d4d638d5fbd735bd for a full test report.

@boegel boegel merged commit 896eb60 into easybuilders:develop Jul 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants