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

Suggestion to add precompile_statements_file_out option #336

Closed
wants to merge 5 commits into from
Closed

Suggestion to add precompile_statements_file_out option #336

wants to merge 5 commits into from

Conversation

IanButterworth
Copy link
Member

For debugging purposes. Just saves the total list to file.
Not sure if helpful in the broader sense. Just a suggestion.

@codecov-io
Copy link

codecov-io commented Feb 14, 2020

Codecov Report

Merging #336 into master will decrease coverage by 0.66%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #336      +/-   ##
==========================================
- Coverage   81.18%   80.51%   -0.67%     
==========================================
  Files           2        2              
  Lines         303      308       +5     
==========================================
+ Hits          246      248       +2     
- Misses         57       60       +3
Impacted Files Coverage Δ
src/PackageCompiler.jl 79.64% <50%> (-0.73%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c45639a...fbedd29. Read the comment docs.

@DilumAluthge
Copy link
Member

@KristofferC What do you think about merging this? It seems like it would make debugging easier.

@KristofferC
Copy link
Member

I am hesitant adding too much API and "scaffolding" around #295. Also, can't you already get these by just running with --trace-compile=output_precompile.jl?

@IanButterworth
Copy link
Member Author

I think you're right that it's a bit much to add to the API.
How about it dumps the precompile statements to a temp file if LLVM errors? The current error you get is a huge wall of red Process(... code, so it might be helpful to make that error nicer at the same time?

@KristofferC
Copy link
Member

KristofferC commented Feb 17, 2020

The precompile files used that contain all the statements should be shown in that red wall though

precompile_statements *= " append!(precompile_statements, readlines($(repr(tracefile))))\n"
end
for file in precompile_statements_file
precompile_statements *=
" append!(precompile_statements, readlines($(repr(file))))\n"

but I agree it might not be very discoverable.

@IanButterworth
Copy link
Member Author

See what you think of the approach now. I don't feel strongly here, but might be an improvement?

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