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

Fix portable shell command arguments in Process#prepare_args #13942

Conversation

GeopJr
Copy link
Contributor

@GeopJr GeopJr commented Nov 2, 2023

fix: #13919

Not sure if the discussion on #13919 is done, but pushing it as a start just in case there are any issues missed

I haven't noticed anything on linux-gnu and was able to compile the compiler with it. BSDs might need more testing if the smoke test is not enough though

Co-authored-by: psykose <alice@ayaya.dev>
@straight-shoota
Copy link
Member

BSDs might need more testing if the smoke test is not enough though

Yeah, smoke tests just cross-compile and don't run anything. So they just make sure the stuff builds, but not that it works as intended.

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

It looks like a great simple patch and it appears to fit for all platforms. So 👍 from me!

@GeopJr
Copy link
Contributor Author

GeopJr commented Nov 2, 2023

FreeBSD

Compiler can compile the compiler but std spec failed with the following:

Screenshot from 2023-11-03 00-40-52

Debugging the first one:

Screenshot from 2023-11-03 00-50-38

it might have to do with crystal being a system package 🤷
image

This is probably the first time std spec ran on FreeBSD since 1.7.3, including PCRE2

I'm not going to run compiler specs, but I'll give running std specs with the system one and on dragonflyBSD a try and update this comment

edit 1: using the 1.7.3 compiler for std_spec also failed with the same ones
edit 2:

DragonFlyBSD

Compiler can compile itself, std_spec gets stuck at some point, I had to prematurely stop it (after multiple runs plus release builds)

image

Debugging the string ones since it's concerning:

image

image

However, running the freebsd iconv workaround works

image

image

The vm doesn't make it easy to debug why it stalls so I'll run the specs that use process manually

  • process_spec passed
  • kernel_spec passed
  • signal_spec passed
  • call_stack_spec passed
  • file_descriptor_spec passed
  • file_executable_spec passed
  • utils_spec passed
  • call_stack_spec passed

@straight-shoota straight-shoota added this to the 1.11.0 milestone Nov 4, 2023
@straight-shoota straight-shoota changed the title Remove bsd specific condition from Process#prepare_args Fix portable shell command arguments in Process#prepare_args Nov 6, 2023
@straight-shoota straight-shoota merged commit 67c85e3 into crystal-lang:master Nov 6, 2023
54 of 55 checks passed
Blacksmoke16 pushed a commit to Blacksmoke16/crystal that referenced this pull request Dec 11, 2023
This pull request was closed.
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.

Crystal::System::Process shell invocation broken on Linux with FreeBSD sh
3 participants