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

Interpreter: Apply shell expansion in ldflags #12094

Merged
merged 3 commits into from Jun 28, 2022

Conversation

mdwagner
Copy link
Contributor

@mdwagner mdwagner commented Jun 5, 2022

#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:
image
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.

@oprypin oprypin changed the title Fix: Shell expansion in Interpreter ldflags Interpreter: Apply shell expansion in ldflags Jun 7, 2022
@straight-shoota straight-shoota linked an issue Jun 8, 2022 that may be closed by this pull request
@@ -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}` }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
lib_flags = lib_flags.gsub(/`(.*?)`/) { `#{$1}` }
# Execute and expand `subcommands`.
lib_flags = lib_flags.gsub(/`(.*?)`/) { `#{$1}` }

Comment on lines 1 to 14
{% 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
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

@cyangle
Copy link
Contributor

cyangle commented Jun 25, 2022

This is great! I can use the interpreter to debug a Kemal hello world server now.

[development] Kemal is ready to lead at http://0.0.0.0:3000
From: bin/server.cr:7:3 <Program>#->:

     2: 
     3: # Matches GET "http://host:port/"
     4: get "/" do
     5:   response = "Hello World!"
     6:   debugger
 =>  7:   response
     8: end
     9: 
    10: Kemal.run

pry> response
"Hello World!"
pry> response = "test"
"test"
pry> continue
2022-06-25 06:25:53 UTC 200 GET / 14889.29ms

@straight-shoota straight-shoota added this to the 1.5.0 milestone Jun 25, 2022
@asterite
Copy link
Member

I think the command expansion should be done somewhere inside Program. It seems the logic is now duplicated. But we can merge this and then unify it.

@straight-shoota straight-shoota merged commit 2e6b788 into crystal-lang:master Jun 28, 2022
@mdwagner mdwagner deleted the fix/12061 branch June 28, 2022 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid option: -v when linking with libcrypto/libssl
5 participants