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

Constants for osdependecies. #3309

Merged
merged 12 commits into from
May 4, 2020
Merged

Conversation

terjekv
Copy link
Contributor

@terjekv terjekv commented Apr 29, 2020

  • The osdependencies relating to ibverbs and openssl are repeated
    ad nauseam in the code. Occasionally with minor mistakes added
    in.
  • ibverbs / infiniband and openssl are the most frequent
    package lists used, but their use is inconsistent as
    packages change name over time.
  • When packages change names, old easyconfigs will stop working
    unless the dependency statement is updated.
  • Moving these lists into constants allows for the lists to be
    updated in one place and at one time.

terjekv added 4 commits April 29, 2020 21:45
  - The osdependencies relating to ibverbs and openssl are repeated
    ad nauseam in the code. Occasionally with minor mistakes added
    in.
  - ibverbs / infiniband and openssl are the most frequent
    package lists used, but their use is inconsistent as
    packages change name over time.
  - When packages change names, old easyconfigs will stop working
    unless the dependency statement is updated.
  - Moving these lists into constants allows for the lists to be
    updated in one place and at one time.
  - OS_PACKAGES is more readable than OS_PKGS.
@terjekv
Copy link
Contributor Author

terjekv commented Apr 29, 2020

Usage: osdependencies = [OS_PKG_IBVERBS_DEV]. For perl, one can change osdependencies = ['openssl', ('openssl-devel', 'libssl-dev', 'libopenssl-devel')] into osdependencies = [OS_PKG_OPENSSL_BIN, OS_PKG_OPENSSL_DEV].

@terjekv
Copy link
Contributor Author

terjekv commented Apr 29, 2020

@boegel :-(

Found printed messages in output of test suite\nERROR: test_easyconfig_constants (test.framework.easyconfigparser.EasyConfigParserTest) File "/tmp/681175494/lib/python2.7/site-packages/easybuild_framework-4.2.1.dev0-py2.7.egg/test/framework/easyconfigparser.py", line 187, in test_easyconfig_constants self.assertTrue(isinstance(val, (string_type, dict)), "Constant value %s is a string or dict" % val)

Does this mean that constants can't be lists? If so, either the test needs to be changed, or I'll have to wrap the list in a dict and then unpack at a later stage, but that just seems like circumventing the intent of the test.

@boegel
Copy link
Member

boegel commented Apr 30, 2020

@terjekv Using a list type value ([1,2,3]) as a constant is a bad idea because lists are mutable, but using a tuple ((1,2,3)) should be fine (and thus changing that check to also accept tuples as constant values is OK).

@boegel boegel added this to the next release (4.2.1?) milestone Apr 30, 2020
terjekv added 3 commits April 30, 2020 14:30
  - pep8 and pycodestyle both missed the line length locally.
  - Fun note with pycodestyle? It helps to read the correct
    config file.
@terjekv
Copy link
Contributor Author

terjekv commented Apr 30, 2020

@terjekv Using a list type value ([1,2,3]) as a constant is a bad idea because lists are mutable, but using a tuple ((1,2,3)) should be fine (and thus changing that check to also accept tuples as constant values is OK).

Blargh. I fail at terminology. Sorry about that. I ment tuple. I have updated the test code. Let's see if that helps.

@easybuilders easybuilders deleted a comment from boegelbot Apr 30, 2020
easybuild/framework/easyconfig/constants.py Outdated Show resolved Hide resolved
easybuild/framework/easyconfig/constants.py Outdated Show resolved Hide resolved
  - Constants are now split into dev, bin, and lib sets.
  - Names are slightly simplified.
@terjekv terjekv requested a review from boegel May 2, 2020 17:47
@easybuilders easybuilders deleted a comment from boegelbot May 2, 2020
terjekv added 2 commits May 2, 2020 21:49
  - Python doesn't like it.
  - Also updated inline comments with regards to acceptable
    types for the values of constants.
@easybuilders easybuilders deleted a comment from boegelbot May 4, 2020
@easybuilders easybuilders deleted a comment from boegelbot May 4, 2020
test/framework/easyconfigparser.py Outdated Show resolved Hide resolved
terjekv added 2 commits May 4, 2020 15:44
  - Based on @boegel's phrasing request, from lessons related to
    the lack of tests not being fully isolated from the
    environment. See the following issue for more information:
    easybuilders#3231
  - Also added back the stringification of the types, including
    the tuple, by wrapping the output in str().
@terjekv terjekv requested a review from boegel May 4, 2020 15:02
@boegel
Copy link
Member

boegel commented May 4, 2020

Thanks a lot @terjekv!

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

Successfully merging this pull request may close these issues.

2 participants