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

CI fixes #1744

Merged
merged 4 commits into from
Apr 6, 2019
Merged

CI fixes #1744

merged 4 commits into from
Apr 6, 2019

Conversation

henryiii
Copy link
Collaborator

Working on fixing the CI failures.

@wjakob
Copy link
Member

wjakob commented Mar 27, 2019

Thanks for taking a stab at this. That said, switching to sudo: true is suboptimal. It sould constrain us to us to Travis legacy, which means that CI takes much longer to run etc.

@henryiii
Copy link
Collaborator Author

henryiii commented Mar 27, 2019

Travis killed container based (sudo: false) builds in December 2018. See the bottom of this page or the blog post last year. So the setting has no effect now and should be removed. It’s the equivalent of sudo: false always now. For faster builds we could always add Azure; for other projects I've tried it on, it's 2-3x faster than Travis and supports all three OSs, sometimes in a single file.

@henryiii
Copy link
Collaborator Author

I’ll get back to working on this after I finish traveling. It’s not easy to work on CI on a plane ;)

@henryiii henryiii force-pushed the henryiii-fixci branch 2 times, most recently from 8e28174 to 726e57d Compare March 27, 2019 10:28
@bstaletic
Copy link
Collaborator

At least build 7 is easy - CLANG should be set to 5.1. I haven't looked at the others in detail, but build 6 is complaining that it can't find python3.6-dev and build 10 doesn't make too much sense to me.

@wjakob
Considering that lately the only merged PRs are those that fix CI and the development of this library has slowed down considerably, would you be willing to invite more contributors as members?
I was thinking about creating an issue where we could state goals of pybind loud and clear and grant a few people who have contributed to this codebase and helped others on gitter merge rights.

@eacousineau
Copy link
Contributor

eacousineau commented Mar 29, 2019

FYI #1725 could be closed pending solution of the python3.6-dev bits (or I guess closed now as this PR is open :P).

For this bit, though, it looks like Ubuntu Trusty may only provide python3.4?
https://packages.ubuntu.com/trusty/python3.4-dev

EDIT: Sorry, didn't see the deadsnakes PPA. Seems like they have python3.6-dev:
https://launchpad.net/~deadsnakes/+archive/ubuntu/ppa/+sourcepub/9799295/+listing-archive-extra
Is Travis still honoring the addons/apt/sources field?

EDIT 2: Is there a reason why Trusty is still being used, when Travis has Xenial available? (I'd be happy to help upgrade.)
I've briefly tinkered with setting up Travis CI here.

Re: @bstaletic's comment, I'd be happy to help there too. Is there another place / issue in which we can discuss this?

@henryiii
Copy link
Collaborator Author

henryiii commented Mar 29, 2019

@eacousineau and @bstaletic

Thanks for the help! I can add you to my fork if you'd like to tinker. I'm working on this off and on. I'll try to use --force-with-lease instead of -f to avoid clobbering any pushes, though retraining my fingers can be hard!

Using language=Python, python=3.6 works on Travis Trusty, but we use CPP. I'm not sure why deadsnakes seems to work in some places but not all (the PPA is still there, and I think there would be a clear error if it was no longer whitelisted by Travis).

I'm checking to see if we can use LLVM 7.

@bstaletic
Copy link
Collaborator

Maybe we should try a different thing for python - pyenv instead of ubuntu packages.
As for LLVM, version 7 installed, but most likely as clang-7 (and similar) so cmake still finds the old one.

@eacousineau
Copy link
Contributor

eacousineau commented Mar 30, 2019

I can add you to my fork if you'd like to tinker [...]

I've just activated TravisCi on my fork, so I'll see if I can refine stuff there to avoid clobbering: eacousineau#2
(Also, just learned about --force-with-lease - thanks!)

UPDATE: At least failures on *.6 (can't find "python3.6-dev") and *.7 are the same (can't find "libstdc++7") on mine. Perhaps C++17 somehow clobbers things?

  • 2: I've always been confused by the Docker dispatch logic... I sprinkled in some set -x in the scripts to confirm, and It seems like there's a mix of both direct Travis CI image usage and docker stuff. It's failing to find python3.6-dev in the docker container, I believe, even though Travis adds the deadsnakes bits.

I'm going to try (a variant of) @bstaletic's suggestion of (pyenv) and switch over to language: python, and perhaps remove some of the docker bits where not necessary.

That being said, I realize now that the PyPy stuff is functionally broken; will try that out of hashing out the other bits.

@eacousineau
Copy link
Contributor

eacousineau commented Mar 30, 2019

Fixed the CPP=17 builds (GCC=7 and CLANG=7) by removing the docker execution shim and tinkering with packages:
henryiii/pybind11@henryiii-fixci...eacousineau:feature/henryiii-fixci

Switching from trusty to xenial may have not been necessary, but meh.

Other changes:

  • Normalized syntax
  • Specified os + dist on each build. If it's not actually common, better to make it explicit?
  • Sprinkled set -ex + make VERBOSE=1 for debugging. Travis's lack of variable expansion makes sense for private keys and all, but makes it hard to actually figure out what's going on.
  • Made DOCKER explicitly part of env. It always confused me as to when it was used.

For now, avoided doing any pyenv or language: python changes.

@wjakob
Copy link
Member

wjakob commented Mar 31, 2019

@bstaletic : I'll try to find some time to merge important PRs in the next weeks, and I agree that a discussion about which ones are important could be useful to have. I am very ambivalent about handing out commit rights to solve a perceived bottleneck at the maintainer level, however. To be honest, many of the 90 open PRs are simply not up to the quality standards of this project.

An important thing to consider is that this is a header file library, where minimizing bloat should be a key criterion both to minimize compile time and object code size in dependent projects. This is also a very mature project -- many of the open PRs add features with limited use for 99% of users of this project, but they would add bloat that affects everyone downstream. So the fact that development has slowed down is also partly a consequence of the maturity and special cost-benefit situation of a header file library.

@bstaletic
Copy link
Collaborator

@wjakob I agree with everything you said, but consider one more thing.

Some PRs are just abandoned, by either the PR author or the maintainers. More specifically, I'm interested in #864, #781 and #1210 . Out of these three #1210 would reduce bloat if one only needs a caster for std::optional and/or std::variant. #864 is interesting (to me) because diving into namespace detail always looked like a dirty hack. Finally, #781 shows significant performance and memory usage gains.

I would have polished up these PR, but so far I decided not to, because I thought they just wouldn't get merged.

Then there's Python 2 end of life nearing and dropping it, though it's debatable whether it should happen as soon as possible after 01.01.2020, would reduce a ton of complexity.

To make my point clear, my request for more maintainers wasn't about merging all the PRs. I, myself, many times said that pybind's goal isn't to be exactly what Boost.Python is. My request was more about allowing PRs that would be really nice to have to finally get merged.

Like I said, I understand the concern about making pybind bloated for the benefits of a tiny minority of users, but I was hoping that could be "fixed" by carefully choosing the new maintainers.

@henryiii
Copy link
Collaborator Author

henryiii commented Apr 1, 2019

@wjakob One example is 2.3.0; it's been a very long time since 2.2.x was released, and only the master branch (so 2.3.x candidate) works with nvcc & CUDA. It would be nice to have someone prepare and release 2.3.0.

@henryiii
Copy link
Collaborator Author

henryiii commented Apr 1, 2019

I'm traveling again to another conference so will revisit sporadically for the next week.

@eacousineau
Copy link
Contributor

@henryiii I'm still ironing out the PyPy CI errors (reverting back to 5.8.0 still yields odd download errors...) If you'd like, once I have that working, I can push a single commit on top of yours?

@wjakob All of that makes sense. It would still be nice to have a list of people who are available to respond promptly to / triage PRs and issues, even if overall development slows down. It would also be excellent to have the "maintenance mode" be made more visible, either through the README / docs / whatever.

At present, CONTRIBUTING says it's run through your spare time, but nothing about the accepted scope of features. Perhaps it'd be better to be more explicit on the roadmap and state that big changes should not happen in this project, but in forks (if they're really necessary?).
(Or, if there are potential plans for a 1.x.y, they should happen in X repository at the risk of instability / bloat / etc.)

[...] many of the 90 open PRs are simply not up to the quality standards of this project [...]

This is fine as well, but it would be excellent to enable people to know what those quality standards are, objectively. CI passing of course, if the design is nailed down. But there could be other things that could facilitate feature development in an iterative fashion - if it's merited. If it conflicts with the roadmap, then it'd be excellent to say as much?

@henryiii
Copy link
Collaborator Author

henryiii commented Apr 3, 2019

@eacousineau, I've added you to my fork. Feel free to push to this PR. Let me know if you need help or are not working on some part, otherwise I'll probably just wait till you push.

@eacousineau
Copy link
Contributor

eacousineau commented Apr 4, 2019

Sorry for the delay, and thanks!
I'm still trying to figure out the last PyPy bits, mostly by bisecting between old stuff and new, pinning versions, etc. If/when I have that working, I'll push a commit here (aiming towards non --force-ful pushes).

EDIT: I think I have the fixes. Will await the WIP build, then push here.

@eacousineau
Copy link
Contributor

Think it's working now: wip branch CI results

Just pushed the commit delta on this branch (added details to the commit).

henryiii and others added 3 commits April 4, 2019 08:08
* Fix warning that not including a cmake source or build dir will be a fatal error (it is now on newest CMakes)
    * Fixes appveyor
* Travis uses CMake 3.9 for more than a year now
* Travis dropped sudo: false in December
* Dropping Sphinx 2
- clang7: Suppress self-assign warnings; fix missing virtual dtors
- pypy:
  - Keep old version (newer stuff breaks)
  - Pin packages to extra index for speed
- travis:
  - Make docker explicit; remove docker if not needed
  - Make commands more verbose (for debugging / repro)
  - Make Ubuntu dist explicit per job
@henryiii
Copy link
Collaborator Author

henryiii commented Apr 4, 2019

Did Travis really add a name field?!?!?!? Wow, yes they did. Finally!

@henryiii
Copy link
Collaborator Author

henryiii commented Apr 4, 2019

We'll need to open an issue for adding PyPy 7 support at some point (outside the scope of this already large PR). Probably should start checking GCC 8 and 9, too; again, in a later PR.

@henryiii
Copy link
Collaborator Author

henryiii commented Apr 4, 2019

@wjakob This passes! Ready for review and merge.

@henryiii
Copy link
Collaborator Author

henryiii commented Apr 6, 2019

Ping? The warning bugs on newer compilers are very irritating, I'd like to get it fixed, as well as it would be very good to fix the PyPy support. No that can happen until the CI starts passing again.

@wjakob
Copy link
Member

wjakob commented Apr 6, 2019

Hi @henryiii,

thank you very much for fixing the CI issues (it looks like it was a lot of work!).

I'll merge this right away.

Best,
Wenzel

@wjakob wjakob merged commit ae951ca into pybind:master Apr 6, 2019
@wjakob
Copy link
Member

wjakob commented Apr 6, 2019

@henryiii : after merging the PR, the AppVeyor build didn't pass due to a deprecation warning.

@henryiii
Copy link
Collaborator Author

henryiii commented Apr 6, 2019

It should be a harmless warning and the transaction finished verification before it excecuted. That's odd. I'll see if I can fix it. I think it started happening recently (the question I found when googling it is a day old). The fix will be released next week. But it still should not kill the install!

@henryiii
Copy link
Collaborator Author

henryiii commented Apr 6, 2019

Note: quite a bit of the work was from @eacousineau too!

@henryiii henryiii deleted the henryiii-fixci branch April 6, 2019 19:40
@henryiii
Copy link
Collaborator Author

henryiii commented Apr 6, 2019

@wjakob Looks like we got unlucky; the PR build was with Conda 4.6.9, which is fine, and the master build was with Conda 4.6.11, which has a bug. I've made a new PR (#1757), which sidesteps the issue by removing the initial conda update (conda still gets updated, but since it all happens at once, the transaction is executed by 4.5.4 and succeeds).

The conda bug should be fixed next week. See conda/conda#8512.

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.

4 participants