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

Test oneliner without double escaping dollar sign #356

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

atoomic
Copy link
Contributor

@atoomic atoomic commented Jul 21, 2020

Fix #355

Double escaping '$' for Makefile usage is fine, but
we should not use such a syntax while testing and trying
to run the oneliner output.

This change is 'unescaping' the '$$' to '$' so we can perform
some extra checks while using oneliner.

Otherwise as shown in this example a simple 'my $foo' test will fail.

@Leont
Copy link
Member

Leont commented Jul 21, 2020

LGTM.

Technically it's wrong if the path to perl contains two dollar signs, moving it one line up would fix that unlikely edge-case.

@atoomic
Copy link
Contributor Author

atoomic commented Jul 21, 2020

good point... was not considering it being an option, but you are right and I moved it above via db19381

@mohawk2
Copy link
Member

mohawk2 commented Jul 22, 2020

Generally I favour this change.

Unfortunately, the CI fail reveals a real problem: dmake has to double up its braces. If you want to unquote them, you'll need to reverse that too.

I feel like try_oneliner is trying to be too clever by trying to de-quote the make quoting, and it's getting it wrong. Surely the right thing to do is create small makefiles that have these recipes, and just use make to run them?

@Leont
Copy link
Member

Leont commented Jul 22, 2020

I feel like try_oneliner is trying to be too clever by trying to de-quote the make quoting, and it's getting it wrong. Surely the right thing to do is create small makefiles that have these recipes, and just use make to run them?

And apparently VMS also does its own unique thing. Writing out a Makefile seems like the way to go.

@atoomic
Copy link
Contributor Author

atoomic commented Jul 22, 2020

nice catch and this comes from the additional test using a hash I've added :-)
let me see if unescaping those braces fixes it

syntax error at -e line 1, near "{{"
Execution of -e aborted due to compilation errors.

#   Failed test '%h and $h'
#   at t/oneliner.t line 42.
#          got: ''
#     expected: '2'
# oneliner:
# "C:\strawberry\perl\bin\perl.exe"  -e "my %h = (1, 2); print $h{{1}}" --
# Looks like you failed 1 test of 17.
[21:25:20] t/oneliner.t .............. 
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/17 subtests 

@atoomic
Copy link
Contributor Author

atoomic commented Jul 22, 2020

going to work on the Makefile solution

@atoomic atoomic force-pushed the oneliner-test branch 2 times, most recently from c0319fa to 74d9a09 Compare July 22, 2020 16:02
@Leont
Copy link
Member

Leont commented Jul 22, 2020

going to work on the Makefile solution

On VMS, the Makefile needs to be called Descrip.MMS AFAIK. Possibly other operating systems have similar quirks.

@atoomic
Copy link
Contributor Author

atoomic commented Jul 22, 2020 via email

@craigberry
Copy link
Member

craigberry commented Jul 22, 2020 via email

Fix Perl-Toolchain-Gang#355

Double escaping '$' for Makefile usage is fine, but
we should not use such a syntax while testing and trying
to run the oneliner output.

We could consider unescpaing '$$', '{{', '}}'... (for dmake)
but a cleaner solution is to test the oneliner in a Makefile
itself.

sub Makefile_template {
my ($RUN) = @_;
my $NOECHO = '@';
Copy link
Member

Choose a reason for hiding this comment

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

This will not be correct on all platforms

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed you are right. This is kind a of a chicken egg issue: producing Makefile to test code which generate Makefile..
What would be your preference?

  • do we have some helpers we could use for this?
  • skipping some distro for this test?
  • going back to the unescape solution?
  • other?

thanks for your valuable input

Copy link
Member

Choose a reason for hiding this comment

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

Skipping and unescaping are not viable approaches. Probably the way to go here is to be making a sandbox dir, then minimal Makefile.PL. Look in the XS tests for functions to make this easy.

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.

oneliner helper does not allow variables
4 participants