Skip to content

Conversation

szmate1618
Copy link

@szmate1618 szmate1618 commented Aug 27, 2022

Fixes #162.

This pull request currently contains 2 changes:

  • croak on empty (or undefined) strings in _search_path
  • croak on references to undefined arrays in harness

Any of the two would be sufficient on its own to solve this specific problem, but I suggest to go with both.

I think the first change is important because regardless of this issue, it is semantically incorrect to search for empty file names on a path. And the second change is important because if we know we are going to fail, it's better to fail fast. Of course, failing really fast would mean the very first line of run, but after studying the code a little, it felt like the correct place to handle such special cases is in harness.

Instead of failing silently I'm using croak in both cases, but I'm unsure if this is the correct decision, and would like to hear your input. I'm unsure, because this library has many users, and it is quite possible that someone has accidentally (or not) written code that depends on run not failing on undefined values. This fix would break such code. On the other hand, if your code accidentally depends on a bug, maybe you want it to break, so you notice the problem.

I would add some tests, but to be perfectly honest, I have no idea how the test framework works.

@szmate1618
Copy link
Author

szmate1618 commented Aug 29, 2022

Ok, I did not expect those test failures. I did run the tests on my machine, at least on Windows, and they passed (except for the ones that are skipped on that platform). edit: I accidentally ran the tests on an earlier version, without my changes.

I will look into this tomorrow.

@nmisch nmisch added the stalled Waiting for response from ticket creator. label Nov 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stalled Waiting for response from ticket creator.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IPC::Run::run() might run executables that are explicitly added to PATH

2 participants