-
Notifications
You must be signed in to change notification settings - Fork 705
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 missing Pandoc dependency for DROP 1.0.3 #12801
add missing Pandoc dependency for DROP 1.0.3 #12801
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.
Is this truly a runtime dep? I would have thought that it maybe was a builddep for documentation or some such?
yes it is, for creating reports, dependency graphs and such. not everyone will need it, but it's just one extra dep so not a big deal, or do you prefer to comment it out? |
Well, it does mean that we end up with amd64 support only, since Pandoc is a binary package. Perhaps best to keep it commented out for this reason? |
toolchain = SYSTEM | ||
|
||
source_urls = ['https://github.com/jgm/pandoc/releases/download/%(version)s/'] | ||
sources = ['%(namelower)s-%(version)s-linux-amd64.tar.gz'] |
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.
Hmm, we should probably use a versionsuffix
here, since they (now?) provide arm64
builds 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.
If so, wouldn't it be better to provide multiple sources here instead?
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.
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.
There's a better option, we can avoid the versionsuffix
for Pandoc, and stick to a single easyconfig file that supports both x86_64
and aarch64
, but we need easybuilders/easybuild-framework#3670 for 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.
Huh, I thought we could do that already? Is it (currently) just implemented as a template variable for a few selected easyblocks (like Java) before?
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 also a bit confused.
a lot of easyconfigs already use %(arch)s
in their sources string, why can't we use that here 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.
is it because we cannot translate %(arch)s
into amd64
or arm64
in the easyconfig?
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.
Hm, I thought we even supported using a dictionary in sources, but maybe that's just dependencies (like Java) or checksums (like CUDA) does?
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.
@Micket changed to use the ARCH constant, now that easybuilders/easybuild-framework#3670 is merged.
Java uses 'arch=xxx': 'yyy'
syntax, but that's indeed only supported for dependencies
other easyconfigs use the %(arch)s
template for sources, but that's only possible if the sources have the exact %(arch)s string in their name, since the template is only resolved after parsing the easyconfig.
…e %(arch)s Pandoc dependency for DROP
add versionsuffix to Pandoc 2.13 to indicate target architecture + use %(arch)s Pandoc dependency for DROP
source_urls = ['https://github.com/jgm/pandoc/releases/download/%(version)s/'] | ||
_archs = {'x86_64': 'amd64', 'aarch64': 'arm64'} | ||
sources = ['%%(namelower)s-%%(version)s-linux-%s.tar.gz' % _archs[ARCH]] | ||
checksums = ['7404aa88a6eb9fbb99d9803b80170a3a546f51959230cc529c66a2ce6b950d4c'] |
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.
We need multiple checksums for x86_64+aarch64
@@ -23,6 +23,7 @@ dependencies = [ | |||
('HTSlib', '1.11'), # for tabix | |||
('SAMtools', '1.11'), | |||
('BCFtools', '1.11'), | |||
('Pandoc', '2.13', '', True), |
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 feel it might still be worth leaving it commented, if it's was a niche usecase, and the fact that our PowerPC friends can't use it still. I don't know the software well enough, so, I'll leave it up to you to decide what's 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.
There's still --filter-deps
to easily opt out of depending on Pandoc
, so I'd leave it in...
Test report by @boegel |
Test report by @boegel |
@boegelbot please test @ generoso |
@boegel: Request for testing this PR well received on generoso PR test command '
Test results coming soon (I hope)... - notification for comment with ID 836686884 processed Message to humans: this is just bookkeeping information for me, |
Test report by @boegelbot |
Test report by @boegel |
Test on |
@boegelbot please test @ generoso |
@boegel: Request for testing this PR well received on generoso PR test command '
Test results coming soon (I hope)... - notification for comment with ID 836966357 processed Message to humans: this is just bookkeeping information for me, |
Test report by @boegelbot |
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.
lgtm
Going in, thanks @smoors! |
(created using
eb --new-pr
)