Skip to content

Add Tkinter sanity check to ASE (intel toolchain, take 2).#5909

Merged
boegel merged 2 commits intoeasybuilders:developfrom
schiotz:20180223201220_new_pr_ASE3110
Feb 26, 2018
Merged

Add Tkinter sanity check to ASE (intel toolchain, take 2).#5909
boegel merged 2 commits intoeasybuilders:developfrom
schiotz:20180223201220_new_pr_ASE3110

Conversation

@schiotz
Copy link
Copy Markdown
Contributor

@schiotz schiotz commented Feb 23, 2018

(created using eb --new-pr)

This PR is supposed to replace PR #5692 which failed its selftest due to a broken Tkinter config. As @boegel had already merged a correct version of this file, I tried to remove it but managed to completely mess up the PR with eb --update-pr. I could not recover, and made this replacement. Sorry about the mess, this will require @boegel to review it again. :-(

@schiotz
Copy link
Copy Markdown
Contributor Author

schiotz commented Feb 23, 2018

@boegel Sorry for having to do this, but I ruined my previous PR (#5692) while trying to fix an error I had made, and adapting to your merged PR #5896. I could not salvage that one, so I had to make a new one.

The similar PR for the foss toolchain (#5691) is was not broken.

@boegel boegel added this to the next release (3.5.2 or 3.6.0) milestone Feb 24, 2018
@boegel
Copy link
Copy Markdown
Member

boegel commented Feb 24, 2018

@schiotz Don't worry about ruining the other PR, it happens. Is there anything we can improve to --update-pr to prevent this from happening in the future? The problem was probably that your branch wasn't synced with the latest develop, maybe we need to add support for eb --pr-sync-develop)?

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
Copy link
Copy Markdown
Member

boegel commented Feb 24, 2018

Test report by @boegel
FAILED
Build succeeded for 2 out of 3 (3 easyconfigs in this PR)
node2316.phanpy.os - Linux centos linux 7.4.1708, Intel(R) Xeon(R) CPU E5-2680 v3 @ 2.50GHz, Python 2.7.5
See https://gist.github.com/1e0e3e3a9b9068ae07ecb85529bf1774 for a full test report.

@boegel
Copy link
Copy Markdown
Member

boegel commented Feb 24, 2018

Test report by @boegel
FAILED
Build succeeded for 2 out of 4 (3 easyconfigs in this PR)
node2111.delcatty.os - Linux centos linux 7.4.1708, Intel(R) Xeon(R) CPU E5-2670 0 @ 2.60GHz, Python 2.7.5
See https://gist.github.com/bfe69551aec87aeb9e146ea9fe45da0f for a full test report.

edit: failed due to changes to freetype dep of matplotlib in #5896, will force rebuild those matplotlib modules first and resubmit test report

}

# make sure Tkinter is available, otherwise 'ase gui' will not work
sanity_check_commands = ["python -c 'import tkinter' "]
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.

@schiotz Typo here, should be import Tkinter (capital T)

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

@schiotz
Copy link
Copy Markdown
Contributor Author

schiotz commented Feb 26, 2018

@boegel Sorry about the typo - that is the danger of submitting PRs to toolchains one cannot use. :-)

Is there anything we can improve to --update-pr to prevent this from happening in the future? The problem was probably that your branch wasn't synced with the latest develop, maybe we need to add support for eb --pr-sync-develop)?

I would not worry about adding this to eb. It is unavoidable that PRs get out of sync with the develop branch, and that may involve conflicts and all kinds of mess. Once you are in that situation, it would be difficult to fix through eb's interface, and would require interfacing to (or duplicating) much of the logic in git. One can always either open a new PR, or use git in the relevant branch. If that is too complicated, then doing the same through eb is not going to be easier.

@boegel
Copy link
Copy Markdown
Member

boegel commented Feb 26, 2018

Test report by @boegel
FAILED
Build succeeded for 1 out of 3 (3 easyconfigs in this PR)
node2113.delcatty.os - Linux centos linux 7.4.1708, Intel(R) Xeon(R) CPU E5-2670 0 @ 2.60GHz, Python 2.7.5
See https://gist.github.com/215f87ab0f70489af8e9b0d6e1e02eea for a full test report.

@boegel
Copy link
Copy Markdown
Member

boegel commented Feb 26, 2018

Test report by @boegel
SUCCESS
Build succeeded for 3 out of 3 (3 easyconfigs in this PR)
node2502.golett.os - Linux centos linux 7.4.1708, Intel(R) Xeon(R) CPU E5-2680 v3 @ 2.50GHz, Python 2.7.5
See https://gist.github.com/011de210c6959ad2357e7f6704a292b5 for a full test report.

@boegel
Copy link
Copy Markdown
Member

boegel commented Feb 26, 2018

Test report by @boegel
SUCCESS
Build succeeded for 3 out of 3 (3 easyconfigs in this PR)
gligar03.gligar.os - Linux centos linux 7.4.1708, Intel(R) Xeon(R) CPU E5-2670 0 @ 2.60GHz, Python 2.7.5
See https://gist.github.com/3af6df3341443ea17c31a0a4b40cd1b0 for a full test report.

@boegel
Copy link
Copy Markdown
Member

boegel commented Feb 26, 2018

Going in, thanks @schiotz!

@boegel boegel merged commit 62d44ca into easybuilders:develop Feb 26, 2018
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