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.

Sign up for GitHub

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

Jump to bottom

Search PATH for "port" command. #3298

Merged
merged 3 commits into from Jan 29, 2015
Merged

Search PATH for "port" command. #3298

merged 3 commits into from Jan 29, 2015

Conversation

todd-a-jacobs
Copy link
Contributor

  • Make the test for the MacPorts port command more flexible by searching the PATH rather than assuming it resides at /opt/local/bin/port. This feature was requested in the line notes for Add support for automatic configuration of llvm-config under MacPorts. #3297.
  • Explicitly test that the port command is really MacPorts. This improves reliability beyond the previous reliance on a canonical filesystem location.
  • Contrary to expectations, port version will return a version string that fails to identify itself as MacPorts. String-matching the interactive prompt is therefore more reliable for validating the executable.

@todd-a-jacobs
Liberalize check for MacPorts *port* command.
99625cb 
- Use *which* to search the PATH for the *port* command, rather than
  checking the canonical installation location.
- See <rubinius#3297> for a discussion
  of the pros and cons of this approach.
@todd-a-jacobs
Validate that *port* command in PATH is MacPorts.
7a37705 
Use a more reliable validation technique to ensure that the executable
found in the PATH is really MacPorts, and not some other file named
"port."
@jc00ke
Copy link
Member

jc00ke commented Jan 28, 2015

I'm not familiar with MacPorts, but is there a reason we need to match the version number?

@todd-a-jacobs
Copy link
Contributor Author

@jc00ke We don't actually care about the version number except as a textual anchor. It just seems more reliable to match an anchored "MacPorts " pattern than just /MacPorts/. The goal is defensive coding, not version extraction.

If we care about the MacPorts version in the future, %x(port version).split.last is certainly an option, too. However, Rubinius really only cares about MacPorts to the extent that we want to query it for filesystem locations. Unless MacPorts introduces breaking changes to its query interface that we need to branch on, it seems unlikely that we would ever care about the MacPorts version information during the rbx configuration process.

@jc00ke
Copy link
Member

jc00ke commented Jan 28, 2015

I didn't think so. Might be more clear just to match /MacPorts/ or, like you have, /^MacPorts/. If the version info isn't used, then we may as well use a simpler regex.

@todd-a-jacobs
Match less of the MacPorts interactive prompt.
29763ea 
- Per @jc00ke, we will not use the semantic version from the interactive
  prompt to anchor the validation.
- Switch to a fixed-string match instead of a regex for performance.
- Relevant comments:
    - <rubinius#3298 (comment)>
    - <rubinius#3298 (comment)>
brixen added a commit that referenced this pull request Jan 29, 2015
@brixen
Merge pull request #3298 from CodeGnome/feature/search_path_for_port_cmd
22a2224 
Search PATH for "port" command.
@brixen brixen merged commit 22a2224 into rubinius:master Jan 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@todd-a-jacobs @jc00ke @brixen