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

Include GA4GH support into the standalone binary (make pack) #1666

Merged
merged 4 commits into from
Aug 1, 2020

Conversation

tskir
Copy link
Contributor

@tskir tskir commented Jul 5, 2020

When I tried building a modified Nextflow version, I noticed that the standalone binary generated by make pack command works fine but does not include support for GA4GH/TES functionality. This PR fixes it.

Signed-off-by: Kirill Tsukanov [email protected]

Copy link
Member

@pditommaso pditommaso left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, however, the GA4GH module adds some extra dependencies we don't want to include the distribution.

What could be done is to create a separate Gradle target to build GA4GH distribution package.

@tskir
Copy link
Contributor Author

tskir commented Jul 8, 2020

OK, thanks for letting me know! I suspected there was a reason it wasn't included, just didn't know why exactly.

If you don't mind keeping this PR open for a day or two, I'll look into it and reimplement as a separate Gradle target

@pditommaso
Copy link
Member

No hurry

When I tried building a modified Nextflow version, I noticed that the
standalone binary generated by `make pack` command works fine but does
not include support for GA4GH/TES functionality. This PR fixes it.

Signed-off-by: Kirill Tsukanov <[email protected]>
@tskir tskir force-pushed the include-ga4gh-in-make-pack branch from a69fd19 to ef3c658 Compare July 28, 2020 17:32
@tskir
Copy link
Contributor Author

tskir commented Jul 28, 2020

@pditommaso I implemented the changes which we discussed (sorry it took me twenty days instead of two!) — now GA4GH support is not included in the self-contained binary by default. There is a separate make target for it, and inside Gradle it is controlled via an optional project property. I think it's less cumbersome than including a whole new Gradle target, which would inevitably cause some significant code duplication.

I additionally tested that make pack and make packGA4GH generate different binaries, and the latter one is slightly larger and is the only one to actually include GA4GH support, as expected.

Please let me know what you think.

@tskir
Copy link
Contributor Author

tskir commented Jul 28, 2020

P. S. On my system, the size of binaries is:

  • make pack = 52.3 MB
  • make packGA4GH = 53.5 MB

So the size increase when including GA4GH support seems to be minimal. So, out of curiosity, what is the reason for not including it by default? Are some of the dependencies which GA4GH support brings along unstable, or are there e.g. some license limitations?

Copy link
Member

@pditommaso pditommaso left a comment

Choose a reason for hiding this comment

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

Thanks, this looks a reasonable solution. The reason to not include the all package is that this feature is still experimental. We may reconsider once it's become mature enough.

@pditommaso pditommaso merged commit e2175d4 into nextflow-io:master Aug 1, 2020
@tskir tskir deleted the include-ga4gh-in-make-pack branch August 1, 2020 16:11
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.

2 participants