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

Update to 2.5.17 + change name of package to winflexbison_installer #2

Open
wants to merge 6 commits into
base: testing/2.5.17
from

Conversation

Projects
None yet
3 participants
@madebr
Copy link
Contributor

madebr commented Mar 8, 2019

Show resolved Hide resolved CMakeLists.txt
@@ -1,7 +1,7 @@
cmake_minimum_required(VERSION 3.0.0)
project(cmake_wrapper)

include(conanbuildinfo.cmake)
include("${CMAKE_BINARY_DIR}/conanbuildinfo.cmake")

This comment has been minimized.

@Croydon

Croydon Mar 8, 2019

Member

Why this change?

This comment has been minimized.

@madebr

madebr Mar 8, 2019

Author Contributor

I use the package development flow when developing the recipes.
When building, this does not copy the sources from self.source_folder to self.build_folder,
so the include requires an explicit path.

This comment has been minimized.

@madebr
Show resolved Hide resolved conanfile.py Outdated
Show resolved Hide resolved conanfile.py Outdated
Show resolved Hide resolved conanfile.py Outdated
Show resolved Hide resolved conanfile.py

self.copy(pattern="COPYING", dst="licenses", src=self._source_subfolder)

self.copy(pattern="*.exe", src=self._fake_package, dst="bin",keep_path=False)

This comment has been minimized.

@Croydon

Croydon Mar 8, 2019

Member

What is self._fake_package about?

Again, please stay with the templates as close as possible. We have way too many recipes as custom logic would be easily maintainable.

This comment has been minimized.

@madebr

madebr Mar 8, 2019

Author Contributor

Instead of copying sources out of the build tree, I lest the package install itself.
Because the install procedure does use bin, include, share subdirectories,
I install to a fake package, from which the files are copied from in package()

This comment has been minimized.

@solvingj

solvingj Mar 12, 2019

Member

I don't understand, maybe i'm missing something. If the project has a proper cmake install method defined, we should be able to just call cmake.install() , and the files should be copied directly to package sub-directories by default. I'm trying to use my imagination, but can't see any advantage to having a fake package directory or function or anything. Are we eliminating a copy of some files, or working around some conflicting filenames or something?

This comment has been minimized.

@madebr

madebr Mar 12, 2019

Author Contributor

I use the fake_package directory because the default install step does not copy the built files to the bin subdirectories.

This comment has been minimized.

@madebr

madebr Mar 12, 2019

Author Contributor

The install rules in the cmake file install the files in the root package folder.
You can either tools.replace_in_file all these lines, or simply copy these files manually after it installed itself. I choose the latter because it is easier.

@@ -1,18 +1,14 @@
#!/usr/bin/env python

This comment has been minimized.

@Croydon

Croydon Mar 8, 2019

Member

templates...

This comment has been minimized.

@madebr

madebr Mar 8, 2019

Author Contributor

conanfile's are not scripts, so the templates should be modified.
Agreed?

This comment has been minimized.

@madebr

def package_info(self):
self.env_info.PATH.append(os.path.join(self.package_folder, "bin"))
bindir = os.path.join(self.package_folder, "bin")

This comment has been minimized.

@Croydon

Croydon Mar 8, 2019

Member

templates...

This comment has been minimized.

@madebr

madebr Mar 8, 2019

Author Contributor

This is way more user friendly.
Templates should be changed.

This comment has been minimized.

@madebr
@Croydon
Copy link
Member

Croydon left a comment

  • please stay with the templates as close as possible. Exceptions for good reasons are allowed but should be kept to a minimum. If you think that things can be better, make pull requests to our templates first.
  • we can't easily rename winflexbison to winflexbison_installer see bincrafters/community#489. We would need to re-apply it to conan-center and deprecate the current one. I'm still not entirely sure we want to do that 🤔

@Croydon Croydon changed the base branch from stable/2.5.16 to testing/2.5.17 Mar 8, 2019


if __name__ == "__main__":

builder = build_template_default.get_builder()
builder = build_shared.get_builder()

This comment has been minimized.

@Croydon

Croydon Mar 8, 2019

Member

What are these changes about?

This comment has been minimized.

@madebr

madebr Mar 8, 2019

Author Contributor

This only provides a binary, no libraries.
So adding builds for all combinations of build_type and compiler does not make sense.

This comment has been minimized.

@solvingj

solvingj Mar 12, 2019

Member

Tbh, this is surprising that this package generates no libs. Indeed if it only produces binaries, _installer might make sense. But, as @Croydon said, it's accepted already, so lets publish under current name for now and discuss rename later.

This comment has been minimized.

@madebr

madebr Mar 12, 2019

Author Contributor

I reverted to the original name.
I also added the tests of flex and bison.
I think winflexbison does not need a library, because winflexbison adds the missing symbols to the generated sources.

This comment has been minimized.

@madebr

madebr Mar 12, 2019

Author Contributor

Note that using flex for on visual studio, requires the --wincompat option.

madebr added some commits Mar 8, 2019

@Croydon Croydon self-assigned this Mar 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.