Code review comment for ~chad.smith/cloud-init:cyaml-loading
Revision history for this message
| Ryan Harper (raharper) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
73416ba
(Get the code!)
On Fri, May 5, 2017 at 8:21 AM, Scott Moser <email address hidden> wrote:
> > From Ryan:
> > Some thoughts on this:
> >
> > 1) we don't want users who apt install cloud-init to also pull down
> python-
> > devel, gcc and have to compile the extension, so please don't change
> package
> > deps here
>
> I don't follow this. In ubuntu at least cloud-init does depend on
> python3-yaml and python3-yaml on libyaml-0-2.
>
> $ apt-cache show python3-yaml | grep Dep
> Depends: python3 (<< 3.6), python3 (>= 3.5~), libc6 (>= 2.14), libyaml-0-2
>
> There is no reason to explicitly depend on libyaml-0-2, and we have a
> fallback
> path in the code.
>
> Why did you think we need python-devel ? This is where tox falls apart
> honestly, when you get into C python extensions. But we already have some
> of those in the test path (pylxd).
>
tox compiling
>
> > 2) this is currently an issue for the tox/venv environment, so let's
> focus on
> > how to enable the SafeLoader library for that path
>
> if we do skipIf, then the package build environment will run though the
> CLoader path, and the tox path would not. We could put in some way of
> failing if the package build environment didn't take the CLoader path.
> That'd ensure that we were getting tests on both paths.
>
> I think that might be over-engineering.
>
> > 3) Once we make (2) an optional testing path (env or parameter can
> trigger the
> > dep install prior to calling tox/venv which triggers the compile during
> pip
> > install of the module IIUC), we can have two travis paths, one without
> the
> > library (representing how things work today) and one *with* the library
> > enabled. This allows us to compare results, unittests all pass, and time
> it
> > takes to complete (some estimate of the improvement it may provide).
> >
> > 4) With results from (3), we may propose changes to the underlying images
> > (server seed, cloud-image seed, etc) which could ensure that the library
> is
> > present in the default cloud images.
>
> I think over-engineering, especially for the size of the gain. Its
> entirely possible that there is overhead of loading c that makes this path
> slower, meaning we'd be doing this work for negative result.
>
> Best case, i think the amount of yaml that cloud-init loads is probably
> small enough to look like noise here.
>
Ideally we'll generate some actual numbers here; if it's not worth it, then
we can dump the whole thing. If it is, ideally we'd have
a path through both w.r.t tox/unittesting; I can't say that it's
over-engineering to want to exercise both paths we might expect
cloud-init users might execute.
>
>
> --
> https:/
> cloud-init/
> Your team cloud init development team is requested to review the proposed
> merge of ~chad.smith/
>
> _______
> Mailing list: https:/
> Post to : <email address hidden>
> Unsubscribe : https:/
> More help : https:/
>