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

Add support for 'hidden=True' easyconfig parameter #1837

Merged
merged 6 commits into from
Aug 4, 2016

Conversation

geimer
Copy link
Contributor

@geimer geimer commented Jul 7, 2016

This complements the '--hidden' command-line option.

@boegel
Copy link
Member

boegel commented Jul 7, 2016

@geimer: build_option('hidden') is also used in process_easyconfig, do we need to check the hidden parameter there too?

Looks tricky at first sight, since it's used before parsing the easyconfig, and I'm not sure it's strictly needed there...

@boegel boegel added this to the v2.9.0 milestone Jul 7, 2016
@geimer
Copy link
Contributor Author

geimer commented Jul 7, 2016

@boegel: Something like hidden = hidden or ec['hidden'] in the if not parse_only: block may do the trick. What do you think?

@geimer
Copy link
Contributor Author

geimer commented Jul 7, 2016

Of course I meant easyconfig['hidden'] instead of ec['hidden'] ;-)

@geimer
Copy link
Contributor Author

geimer commented Jul 7, 2016

@boegel: I believe the easyconfig update for hidden in L1230 isn't necessary. The build_option is passed to the EasyConfig constructor, which will correctly initialize the value based on build_option and the hidden easyconfig parameter. Does that make sense?

@geimer
Copy link
Contributor Author

geimer commented Jul 8, 2016

@boegel: Still trying to understand the code...

Shouldn't the update in https://github.com/geimer/easybuild-framework/blob/5d9a56a543eb7f8e078d6aa737077cfc23be40ed/easybuild/framework/easyconfig/easyconfig.py#L1230 be 'hidden': ec.hidden? Then the first query to build_option('hidden') only affects the cache lookup, which should be OK as the data in the cache should already include parsed values. The update then takes the actual parsed value into account, determined in the EasyConfig constructor based on the build option and the easyconfig parameter.

The thing is that the easyconfig unit tests all pass, regardless of what the update does. I can even hard-code it to True or False...

BTW: There is a docstring for the parse_only parameter missing.

@boegel
Copy link
Member

boegel commented Jul 11, 2016

@geimer I think you're right with your last comment. Up until now it didn't really matter, but with the introduction of the hidden easyconfig parameter, it does matter.

I'd have to dive in and figure out where that 'hidden' key is actually used, if at all, it may be "dead code", but it certainly doesn't hurt to get it right (and using ec.hidden is correct imho).

The cache aspect is a bit weird, but only taking into account --hidden in the cache key is correct imho, since the hidden easyconfig parameter only comes into play when the easyconfig is already parsed.

@geimer
Copy link
Contributor Author

geimer commented Aug 4, 2016

@boegel: IMHO, this is ready for review.

@boegel
Copy link
Member

boegel commented Aug 4, 2016

@geimer looks good, but I couldn't resist making a few minor tweaks, see geimer#8

fix help message for 'hidden' + use available auto-cleaned tmp dir in test
@boegel
Copy link
Member

boegel commented Aug 4, 2016

Going in, thanks @geimer!

@boegel boegel merged commit 50f81be into easybuilders:develop Aug 4, 2016
@geimer geimer deleted the hidden_easyconfig branch August 5, 2016 07:49
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.

2 participants