-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Interpreter: Apply shell expansion in ldflags #12094
Conversation
@@ -357,7 +357,10 @@ class Crystal::Repl::Context | |||
end | |||
|
|||
getter(loader : Loader) { | |||
args = Process.parse_arguments(program.lib_flags) | |||
lib_flags = program.lib_flags | |||
lib_flags = lib_flags.gsub(/`(.*?)`/) { `#{$1}` } |
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.
lib_flags = lib_flags.gsub(/`(.*?)`/) { `#{$1}` } | |
# Execute and expand `subcommands`. | |
lib_flags = lib_flags.gsub(/`(.*?)`/) { `#{$1}` } |
{% skip_file if flag?(:without_interpreter) %} | ||
require "./spec_helper" | ||
require "../loader/spec_helper" | ||
|
||
describe Crystal::Repl::Interpreter do | ||
context "openssl" do | ||
it "can require" do | ||
interpret(<<-CR, prelude: "prelude") | ||
require "openssl" | ||
OpenSSL::SSL::Context::Server.new | ||
CR | ||
end | ||
end | ||
end |
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.
This spec tests the changed behaviour only coincidentally. It depends on the bindings and link flags for LibSSL
.
It would be better to have a dedicated test specifically for this command expansion in interpreter/lib_spec.cr
.
Let me know if you need help with 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.
Ah ok. I had the test originally in lib_spec, but thought it was a little different. I'll add it back.
Can you elaborate a little bit on how I can test the command is expanding? I'm just wondering if its a simple as checking if echo ...
worked, or you had something else in mind?
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, I think a simple example of a lib type with bindings specified via shell expansion in ldflags
should be fine. If that links / executes, we can be sure that it works.
We're already using a custom libsum
in lib_spec
for testing variadic arguments. This lib should be available and the symbols are known and consistent. We can probably just derive from one of the existing samples, replacing -lsum
with "-l`echo sum`"
(for example).
Optionally adding trivial function to the lib because we don't need variadics here; but it's probably fine to just use an existing symbol if you like.
This is great! I can use the interpreter to debug a Kemal hello world server now.
|
I think the command expansion should be done somewhere inside |
#12061
I tried following what the compiler does, and saw Windows does this too (https://github.com/crystal-lang/crystal/blob/master/src/compiler/crystal/compiler.cr#L346), so I think this will work now.
However, I get this error on an Intel Mac when trying to run the test file from the issue:
But it seems that might just be an issue with BigSur and/or openssl? The spec seems to properly expand now and that passed locally, so maybe its just an issue with my machine?UPDATE: I was able to make a better spec that properly tests the original issue. Although, I think the issue I was experiencing above is specific to my machine.