Skip to content

Make RubyGem extension EB use GEM_ envs except if installed with Ruby itself#1247

Merged
boegel merged 3 commits intoeasybuilders:developfrom
hajgato:ruby
Dec 27, 2017
Merged

Make RubyGem extension EB use GEM_ envs except if installed with Ruby itself#1247
boegel merged 3 commits intoeasybuilders:developfrom
hajgato:ruby

Conversation

@hajgato
Copy link
Copy Markdown
Collaborator

@hajgato hajgato commented Sep 15, 2017

If I understood correctly, GEM_PATH and GEM_HOME do not have to be set if extensions installed from Ruby. At the old EB, GEM envs were set only if individual gems were installed, not as a bundle. For some reason make_module_extra(self) part does not work (also in the previous version)


# this is the 'proper' way to specify a custom installation prefix: set $GEM_HOME
if not self.is_extension:
if self.cfg.__dict__.get('software_name') != 'Ruby':
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a bit dirty, but I think the real question we should ask is why the existing check based on self.is_extension doesn't, it should, see https://github.com/easybuilders/easybuild-framework/blob/master/easybuild/framework/extensioneasyblock.py#L88

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think it does not work, because it is a Bundle. So the gems are installed as extension not as an EasyBlock (rubygem). But I might misunderstand the code.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@boegel So how to proceed here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK, I think I see the problem...

We need to have $GEM_HOME set both when installing individual Ruby gems as stand-alone modules, and also when installing a bundle of gems together, but not when installing extensions as a part of a Ruby installation (i.e. a "batteries included" Ruby installation).

So, I think we need to keep the not self.is_extension condition, but combine it with another condition via or, like this:

# define $GEM_HOME for both stand-alone installations and bundles of gems,
# to ensure that they are being installed in the right location (i.e. not in parent the Ruby installation)
if not self.is_extension or self.master.name != 'Ruby':
    env.setvar('GEM_HOME', self.installdir)

To clarify: self.master is made available so extensions can access the parent software (in this case, the bundle that is being installed), see https://github.com/easybuilders/easybuild-framework/blob/master/easybuild/framework/extension.py#L57.

Can you give that a try and see if it works as expected?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@hajgato Ping on this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@boegel We have a maintenance period right now, so I will test it when we are online again.

if self.cfg.__dict__.get('software_name') != 'Ruby':
txt += self.module_generator.prepend_paths('GEM_PATH', [''])
return txt
return super(RubyGem, self).make_module_extra(txt)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this actually change anything? What's there should work...

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think the original code was simply not executed. This change, indeed does not make any change, I just simply copied this part from the rpackage EasyBlock.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well, it's wrong as it is now, since we already call out to super(RubyGem, self).make_module_extra() when defining txt, see above.

It should be reinstated to just return txt imho.

@boegel boegel added this to the 3.4.1 milestone Sep 15, 2017
@boegel boegel modified the milestones: 3.4.1, 3.5.0 Oct 17, 2017
@hajgato
Copy link
Copy Markdown
Collaborator Author

hajgato commented Oct 27, 2017

@boegel Ping? Can we do something, or this PR is completely dead?

@boegel
Copy link
Copy Markdown
Member

boegel commented Oct 27, 2017

@hajgato Sorry, I've been meaning to get back to this but keep getting distracted. My plan was to get this merged for EasyBuild v3.4.1, but other stuff got in between...

@boegel boegel modified the milestones: 3.5.0, next release Dec 11, 2017
@hajgato
Copy link
Copy Markdown
Collaborator Author

hajgato commented Dec 18, 2017

@boegel This works perfect.

Copy link
Copy Markdown
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

lgtm

@boegel boegel merged commit 8972b1d into easybuilders:develop Dec 27, 2017
@boegel
Copy link
Copy Markdown
Member

boegel commented Dec 27, 2017

Thanks for the fix @hajgato!

@hajgato hajgato deleted the ruby branch February 12, 2018 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants