Skip to content

Add big-endian s390 architecture on travis#9663

Merged
bsipocz merged 2 commits intoastropy:masterfrom
mhvk:travis-try-s390
Mar 21, 2020
Merged

Add big-endian s390 architecture on travis#9663
bsipocz merged 2 commits intoastropy:masterfrom
mhvk:travis-try-s390

Conversation

@mhvk
Copy link
Contributor

@mhvk mhvk commented Nov 24, 2019

Hopefully I disabled everything else. (EDIT, now ready for real review)

This follows the suggestion of @olebole to try adding the big-ending s390 architecture to our tests. It relies on all our build-dependencies being available via apt.

@mhvk mhvk force-pushed the travis-try-s390 branch 7 times, most recently from c9194cc to cb512b6 Compare November 24, 2019 19:26
@olebole
Copy link
Member

olebole commented Nov 24, 2019

@mhvk I am afraid that this may be not that simple: It is usually not a good idea to mix packages between distributions or even releases of the same distribution; often enough this ends in an unusable system.
One only reliable way would be to install f.e. Debian testing (or unstable) in a chroot and to use that. However, even then it may happen that one or the other new requirement is not available yet.
Another way would be to install the remaining packages via pip from source.

@mhvk mhvk force-pushed the travis-try-s390 branch 3 times, most recently from 698f1eb to 36274af Compare November 24, 2019 20:09
@mhvk
Copy link
Contributor Author

mhvk commented Nov 24, 2019

@olebole - hmm, maybe true, but since Ubuntu is just Debian with a bit of veneer, little of which is relevant for running code, it must be possible! And after about a dozen trials, indeed it is! (Though ideally I would not rely on unstable, but we need updated python3-pytest-doctestplus). And it already recovered some of the same errors you found (fix included).

To me, it seems this would be good enough to put in a cron job at least - let me ping @bsipocz

@mhvk mhvk added no-changelog-entry-needed Affects-dev PRs and issues that do not impact an existing Astropy release Bug units wcs and removed Experimental Work in progress labels Nov 24, 2019
@mhvk mhvk added this to the v4.0 milestone Nov 24, 2019
@mhvk
Copy link
Contributor Author

mhvk commented Nov 24, 2019

@bsipocz - obviously, the two commits with fixes can be pulled out of this PR! Though I do think there is little reason not to test on s390 as well.

@mhvk mhvk requested a review from bsipocz November 24, 2019 20:29
@mhvk
Copy link
Contributor Author

mhvk commented Nov 24, 2019

@olebole - the other reason I like adding this is that it would be good to test a completely apt-based installation; it is no less valid an environment that one using pip or conda.

@mhvk
Copy link
Contributor Author

mhvk commented Feb 21, 2020

I've moved the milestone to 4.1 - no real hurry here.

@mhvk mhvk force-pushed the travis-try-s390 branch 4 times, most recently from db87351 to 173a3d1 Compare February 21, 2020 22:25
@bsipocz
Copy link
Member

bsipocz commented Feb 21, 2020

There are some errors for installing optional dependencies, but otherwise it seems to work nicely, and the total job runtime of 8 minutes is not bad!

https://travis-ci.org/astropy/astropy/jobs/653658052#L966

@mhvk
Copy link
Contributor Author

mhvk commented Feb 22, 2020

Yes, I finally got it going again... Not having setup.py test was interesting - I was a bit disappointed that I seemed to need to create a virtual environment, but perhaps this is because of the extra packages that pip still installed (am trying to not let it install anything, so it also becomes a test of a non-pip install). Anyway, needs a bit more work, but at least getting there.

@bsipocz
Copy link
Member

bsipocz commented Feb 22, 2020

With @pllim excellent detective work, it seems that adding wheel as a dependency will probably solve those build issues, too. Yet, I'm not sure how not to install them, and also I have no idea yet how to make the build fail with those install issues, I would have assumed that those should not let the install and the test run.

@pllim
Copy link
Member

pllim commented Feb 22, 2020

The weird thing is wheel is specified at

"wheel",

so why is it missing?

@pllim
Copy link
Member

pllim commented Feb 24, 2020

Still has the bdist_wheel error, but also weird re.compile error from IPython.

@mhvk mhvk force-pushed the travis-try-s390 branch 2 times, most recently from 7e08f53 to bbe8e40 Compare March 21, 2020 15:18
@mhvk mhvk force-pushed the travis-try-s390 branch from bbe8e40 to 7a37c7b Compare March 21, 2020 18:16
@mhvk
Copy link
Contributor Author

mhvk commented Mar 21, 2020

Hurray, tests now pass on s390! Which brings up the question of what this should replace. In a way, by design it is somewhat orthogonal already to everything that is there, no only testing big-endian (main purpose) but also testing that standard debian (and therefore ubuntu) packages would do the trick.

But I do not want to increase the number of tests. Looking at the present set, we test all dependencies four times:

  1. Initial tests - python 3.6 with all optional dependencies
  2. Comprehensive tests - python 3.6 with all oldest optional dependencies
  3. Comprehensive tests - python 3.7 with all optional dependencies
  4. Final tests - windows python 3.7 with all optional dependencies

This seems too much and my suggestion would be to move all of these "one up" (deleting the first), i.e., have an initial test with all oldest supported versions (most likely to fail), comprensive tests on linux/windows, and final tests as they are.

Alternatively/additionally, I could move this new test to the cron section.

@bsipocz - what do you think? If it is easier, I can just move it to cron here and open a separate issue about reducing the number of tests generally.

@bsipocz
Copy link
Member

bsipocz commented Mar 21, 2020

OK, so I would do is to indeed remove the no1, and replace it with no3 but changing the python version to 3.8. I feel we have more breakage due to upstream updates rather than with old versions, so would keep the most up to date one in the initial set.

As to whether cron or not cron, I don't have strong preference. If we expect this new job to fail on ordinary PRs, then it should stay in the main matrix, but if breakage is infrequent (like for the osx one), then a cron is sufficient.

I don't think we need another issue for lowering the number of builds, we gone through it a few times as it's not really possible to squeeze them any more.

@mhvk
Copy link
Contributor Author

mhvk commented Mar 21, 2020

@bsipocz - decided it made more sense to just move the stage to cron for s390 - I do think it is unlikely to uncover problems with most PRs, so cron seems reasonable. It also means this PR remains nicely self-contained. I skip CI for this last commit, since the first one showed that tests passed.

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

Looks good!

Do you plan to reschuffle the rest of the jobs as we discussed above? or would you mind me to push a commit here that does it here rather than opening another minimal PR?

@bsipocz bsipocz dismissed pllim’s stale review March 21, 2020 22:18

Review has been addressed.

@bsipocz bsipocz merged commit 2509603 into astropy:master Mar 21, 2020
@bsipocz
Copy link
Member

bsipocz commented Mar 21, 2020

Thanks @mhvk!

@bsipocz bsipocz modified the milestones: v4.1, v4.0.1 Mar 21, 2020
@bsipocz
Copy link
Member

bsipocz commented Mar 21, 2020

I'll try to backport this as a non cron job for the LTS release to help ensure we don't break anything there, too. I'm cherry-picking the commit and make the adjustments instead as part of #10059

@bsipocz bsipocz modified the milestones: v4.0.1, v4.1 Mar 21, 2020
@mhvk
Copy link
Contributor Author

mhvk commented Mar 21, 2020

Thanks, let me know if the backport proves troublesome - I'll keep #9670 around just to have a sense how the override used to look like before tox.

@mhvk mhvk deleted the travis-try-s390 branch March 21, 2020 22:29
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.

4 participants