-
Notifications
You must be signed in to change notification settings - Fork 101
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 a PackageCompilerLib plugin #299
base: master
Are you sure you want to change the base?
Add a PackageCompilerLib plugin #299
Conversation
Codecov Report
@@ Coverage Diff @@
## master #299 +/- ##
===========================================
- Coverage 94.42% 73.91% -20.51%
===========================================
Files 20 21 +1
Lines 610 648 +38
===========================================
- Hits 576 479 -97
- Misses 34 169 +135
Continue to review full report at Codecov.
|
I just noticed #290, which hasn't been reviewed yet. There will be conflicts between these, which I can of course fix if that one is reviewed and merged. Is there interest in either of these? |
lib_name=nothing, | ||
build_jl="$(contractuser(default_file("build", "build.jl")))", | ||
generate_precompile_jl="$(contractuser(default_file("build", "generate_precompile.jl")))", | ||
additional_precompile_jl="$(contractuser(default_file("build", "additional_precompile.jl")))", |
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.
What's the use case for this additional_precompile file?
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.
generate_precompile.jl
runs code which is traced to determine which functions to precompile.
additional_precompile.jl
contains explicit precompile statements for compiling versions of functions that were not captured by generate_precompile.jl
Both files exist under the templates/build
directory.
Do you need/want me to add any of this description to the PR?
# glob here in the prehook, so it can be added by gitignore() later. | ||
pkg = basename(pkg_dir) | ||
library_name = lib_name(p, pkg) | ||
push!(p.additional_gitignore, "/$(library_name)-*") |
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 workaround the main purpose of having the additional_gitignore
file? It might be simpler to just force a default library name (e.g. MyLibrary).
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.
Yes, this is the main purpose. Your suggestion is certainly easier--I was just trying to have a complete solution out of the box.
I can remove the additional_gitignore
member if you prefer.
ad1926a
to
cf65bf7
Compare
@mjram0s @Wynand thank you for reviewing, and sorry not to get back to this sooner. I addressed most of the comments, and left a few questions. I have one more change to push to remove the Makefile line (the moment the PackageCompiler PR is merged), plus whatever else you want me to do here. Cheers! |
* This generates files which facilitate creating C libraries from Julia code, using PackageCompiler.jl.
cf65bf7
to
978af1a
Compare
This generates files which facilitate creating C libraries from Julia code, using PackageCompiler.jl.
The MR there (JuliaLang/PackageCompiler.jl#490) actually hasn't been merged there yet, but should be very soon. I'll also be presenting this functionality at JuliaCon, and am hoping/planning to use PkgTemplates as part of this presentation.
It probably shouldn't be merged until the corresponding PackageCompiler.jl code is merged, so I'm marking it as a draft. However, I would love for it to be reviewed, if someone has time.