Skip to content
This repository has been archived by the owner on Jan 30, 2024. It is now read-only.

Fix installer for ruby bindings #212

Merged
merged 1 commit into from Jan 28, 2020
Merged

Fix installer for ruby bindings #212

merged 1 commit into from Jan 28, 2020

Conversation

fao89
Copy link
Member

@fao89 fao89 commented Jan 28, 2020

Bug:

if someone does:
pbindings pulp_file ruby
it will generate ruby bindings, try to pip install it, and it won't install ruby gem.
This PR fixes this bug

@fao89 fao89 requested a review from bmbouter January 28, 2020 17:10
@fao89
Copy link
Member Author

fao89 commented Jan 28, 2020

@bmbouter do you think we have to install rubygems ruby-devel also?

@bmbouter
Copy link
Member

@fabricio-aguiar yes since you have the fix available let's merge it.

Copy link
Member

@mikedep333 mikedep333 left a comment

Choose a reason for hiding this comment

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

See the comment about special vars before merging.

@@ -15,6 +15,7 @@
- tree
- wget
- gnupg
- rubygems
Copy link
Member

Choose a reason for hiding this comment

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

FYI: I tested this by running in a debian:10 container "apt-get install rubygems". It maps to installing the "ruby" package which includes the gem command.

Comment on lines +187 to +191
pip install ./$1-client
;;
ruby)
cd $1-client
gem build $1_client
Copy link
Member

Choose a reason for hiding this comment

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

Are these a safe way to concatenate strings to these bash special variables? I know that bash variables can at least have underscores in them, but special vars (like those beginning with numbers) may follow different rules. If not safe, add the curly brackets.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tested locally(fedora 30 box) and it worked

Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

Thanks!

@fao89 fao89 merged commit c16114d into pulp:master Jan 28, 2020
@bmbouter
Copy link
Member

@fabricio-aguiar can you update pulplift to use this commit also?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants