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

[discussion] Should we create new projects for installer packages #625

Closed
uilianries opened this issue Jan 29, 2019 · 18 comments
Closed

[discussion] Should we create new projects for installer packages #625

uilianries opened this issue Jan 29, 2019 · 18 comments
Labels

Comments

@uilianries
Copy link
Member

Hi everyone!

I've opened this discussion to talk about work overload involving package installers.

We have some packages that could be distributed as installer for binaries only and as regular package for libraries e.g. bison, flex, protobuf. However, when we need to update a version or apply a fix, we need to open a PR for both projects. Maybe we could find some better way to optimize our time and save some CI jobs.

I have some suggestions:

  • Keep the same, continue creating separated packages installer/regular. Two packages to be maintained
  • Use installer as a package option, where it will install only binaries or libraries, and the package id will be changed according the option.
  • Add two recipes on same repo and distribute separated packages.

I would like to know if we are on the right track, or we should rethink about our installer packages.

/cc @Croydon @SSE4 @madebr @solvingj @danimtb @jgsogo

@danimtb
Copy link

danimtb commented Jan 29, 2019

Using the installer as an option would mean that to build that package you would need the ancient glibc and build it in the CentOS image. That would be an additional entry in the build matrix so no time saving in CI.

However, having everything together is an advantage... But what about the settings of the recipe (os_build, os_arch)? Well, they could be deleted in configure()

My conclusion: It could be ok just having an option installer=True making sure the installer has a binary package available for each OS

@theodelrieu
Copy link

What about having os_build, arch_build and all the other settings? I use this combination in the NDK recipe

By adding self.info.include_build_settings() in package_id(), you'd get different packages just like with an installer option. (You'd also get an annoying Conan warning...)

I'm not sure to see the benefits of such an option, except having less packages to store/download.

@uilianries
Copy link
Member Author

I'm not sure to see the benefits of such an option, except having less packages to store/download.

The advantage is maintaining one package less when we need to update a version or apply a fix.

@theodelrieu
Copy link

What about the solution I proposed? It has the same benefits IIUC.

@Croydon
Copy link
Member

Croydon commented Jan 29, 2019

I still thinking that having a separate installer package is

  • confusing for consumers
  • harder to maintain for us
  • and can also bring us a lot of troubles as the upstream projects most of the time aren't meant to get split in such a way (see the Protobuf/Protoc_installer CMake target problem for instance, or the discussions and confusion which packages should be installer packages or not)
    • when it is meant to be split the maintainers are often distributing the parts split by themselves under names like lib-<upstream-name> and <upstream-name>

I would favour a solution which is ending the forced separation from our side.


What about having os_build, arch_build and all the other settings? I use this combination in the NDK recipe

Kinda off-topic: I would say that deleting this settings instead of setting them to ANY is a cleaner approach

@madebr
Copy link
Contributor

madebr commented Jan 29, 2019

As a RFC for having both recipes in one repo, I have created a pr in the flex repo: bincrafters/conan-flex#4

@jgsogo
Copy link
Member

jgsogo commented Jan 30, 2019

I really like the idea of having the installer and the library in the same github repository, I think it is easier to keep both recipes syncronized and up to date, although it will lead to longer CI times.

Also, I would find it quite useful to add an integration folder with tests that contains a crosscompiling example using both packages: like in the protobuf one, if the installer is using the library to compile itself it could lead to a tricky dependency graph that it is worth testing (I think @uilianries knows something about it 😄 )

Thanks for sharing your concerns about it.

@uilianries
Copy link
Member Author

I'll try a POC with both approaches, thanks everyone for sharing your opinion.

@danimtb
Copy link

danimtb commented Jan 30, 2019

I was thinking about this and the possible conflicts you could have using the same package as a requirement (to use the library) and as build require (to use the tool/installer), and I am not sure if you can mix different versions of that same package.

Moreover, I still think the right approach is to have them separated, although this does not mean that that would be the only way to do it.

As a way to prove the points above, I would like to do a test with Protobuffers package with both approaches.

@uilianries
Copy link
Member Author

Related RFC: bincrafters/conan-flex#4

@madebr
Copy link
Contributor

madebr commented Jan 31, 2019

RFC using python_requires: bincrafters/conan-flex#5
(non-working because It shows a shortcoming of python_requires)

@madebr
Copy link
Contributor

madebr commented Feb 19, 2019

Bison has received a treatment as well: bincrafters/conan-bison#4

@SSE4
Copy link
Member

SSE4 commented Apr 4, 2019

I am closing this one now. I have implemented bison/3.3.2@bincrafters/stable and bison_installer/3.3.2@bincrafters/stable within the same repository. it could be used as [RFC] for any further installers (I believe, ICU, vulkan, flex).

@SSE4 SSE4 closed this as completed Apr 4, 2019
@madebr
Copy link
Contributor

madebr commented Apr 4, 2019

@SSE4
Cool!
I am running into a problem with the flex PR because flex depends on flex_installer.

@madebr
Copy link
Contributor

madebr commented Apr 4, 2019

@SSE4
The exports in bison_base.py is overridden by the exports in conanfile.py and bison_installer.py.

@madebr
Copy link
Contributor

madebr commented Apr 4, 2019

@SSE4
Also, the tests in test_package is not testing the package at all.

@SSE4
Copy link
Member

SSE4 commented Apr 4, 2019

@madebr

  1. I'll take a look into the flex later. I'm gonna check ICU first, as it's needed by few other packages.
  2. yeah, seems to be a left-over, base class doesn't need exports, I'll remove em
  3. it links with liby (bison library) and uses symbol it provides (yyerror). there's only one symbol/function bison exposes https://docs.oracle.com/cd/E19683-01/816-0218/6m6nirqnb/index.html

@SSE4
Copy link
Member

SSE4 commented Apr 4, 2019

@madebr
2. update - fixed at bincrafters/conan-bison@c2c2a96

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

No branches or pull requests

7 participants