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

Merge the webio dep code in Blink #174

Closed
wants to merge 6 commits into from
Closed

Conversation

ranjanan
Copy link
Contributor

@ranjanan ranjanan commented Nov 3, 2018

Currently WebIO uses Requires.jl to interface with Blink. However, we currently can't statically compile packages that use Requires.jl.

This helps support for shipping statically compiled Blink apps.

@ranjanan
Copy link
Contributor Author

ranjanan commented Nov 6, 2018

@pfitzseb or @MikeInnes could you please review? Thank you.

@pfitzseb
Copy link
Member

This looks generally fine, but it might be better to use the code in WebIO#master and require WebIO@0.5 instead (that release will probably be out today).
Would you mind also preparing a PR to WebIO to remove the Blink integration over there?


If this is time critical we could also merge this as is and release a new Blink version before WebIO is updated though (but then you should add an upper bound on WebIO).

@pfitzseb
Copy link
Member

@NHDaly
Copy link
Collaborator

NHDaly commented Nov 17, 2018

Thanks for looking into this, Ranjan!

So I ran into this problem from here:
https://github.com/JuliaGizmos/WebIO.jl/blob/116956c4621c3cd18ff0689f8b86817d550dfa7b/src/WebIO.jl#L65-L80

Is that the same as what you're referring to?
Even after this PR, won't you still have the same problems with the other packages it @requires?

That said, I still think this PR is still beneficial! It seems weird to have Blink specifically logic conditionally defined in WebIO if we could just have it here instead.

@NHDaly
Copy link
Collaborator

NHDaly commented Nov 17, 2018

EDIT: nvm apparently they need to be in the __init__ method. See next comment.

(Regarding the static compilation, i think the other part of the problem is that those lines are in the __init__ function. Is there any reason for that? Couldn't they be top-level statements? If they were, I think we could fix this problem by just making sure to import all those modules during the static compilation phase.

Maybe we should open a PR over there to move them out of __init__?)

@NHDaly
Copy link
Collaborator

NHDaly commented Nov 17, 2018

JK please ignore the above:

https://github.com/MikeInnes/Requires.jl:

@require must be within the init method for your module. Here's an example that will create a new method of a function called myfunction only when both packages are present:

:(

@ranjanan
Copy link
Contributor Author

Apologies for not getting to this sooner, but I have made the relevant PR on WebIO, so we could potentially merge this and JuliaGizmos/WebIO.jl#232 and tag new releases for both packages.

@twavv
Copy link
Member

twavv commented Jun 14, 2019

This was usurped by #201 and should be closed.

@pfitzseb pfitzseb closed this Jun 15, 2019
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.

4 participants