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

add lzfse/1.0 #890

Merged
merged 1 commit into from
Feb 24, 2020
Merged

add lzfse/1.0 #890

merged 1 commit into from
Feb 24, 2020

Conversation

SpaceIm
Copy link
Contributor

@SpaceIm SpaceIm commented Feb 16, 2020

Specify library name and version: lzfse/1.0

  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

#621

@conan-center-bot
Copy link
Collaborator

Some configurations of 'lzfse/1.0' failed in build 1 (275ea61c1a3c548640769072bd3c647ae2d8f16f):

@conan-center-bot
Copy link
Collaborator

All green in build 2 (af958a18ff79ec10266482f6003b98385b8548a2)! 😊

SSE4
SSE4 previously approved these changes Feb 17, 2020
@SSE4 SSE4 requested a review from uilianries February 17, 2020 14:51
@uilianries
Copy link
Member

@SpaceIm please, consider SpaceIm#5

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Feb 17, 2020

@uilianries

  • If I understand, it is preferred to also build exe if available, so that the package can also be used as a build requirement?
  • except for POSITION_INDEPENDENT_CODE TRUE, modifications in patch file seems to be useless (LZFSE_BUNDLE_MODE is False). Therefore, is not a better solution to use replace_in_file, since patches can be painful to maintain across versions.

@uilianries
Copy link
Member

If I understand, it is preferred to also build exe if available, so that the package can also be used as a build requirement?

Yes. As that package is more about the library and not the executable, it doesn't require an extra package only for the installer.

except for POSITION_INDEPENDENT_CODE TRUE, modifications in patch file seems to be useless (LZFSE_BUNDLE_MODE is False). Therefore, is not a better solution to use replace_in_file, since patches can be painful to maintain across versions.

No, because it forces BUILD_SHARED_LIBS.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Feb 17, 2020

When LZFSE_BUNDLE_MODE is False, BUILD_SHARED_LIBS is an option, therefore it's not forced.

@uilianries
Copy link
Member

Sorry, indeed it's an option. I need to update that patch.

@uilianries
Copy link
Member

@SpaceIm , done, please take a look again on that PR

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Feb 17, 2020

@uilianries, last part of the patch file is also useless. You can keep the original replace_in_file I think, and remove references to patch file in conandata.yml and conanfile.py.

@uilianries
Copy link
Member

hmm, that's true too. @SpaceIm please, ignore that patch and call cmake.install() instead.

@uilianries
Copy link
Member

Thanks @SpaceIm !

@conan-center-bot
Copy link
Collaborator

All green in build 3 (edf9a9af864ecd5ba9e2f73b905c392cc7088426)! 😊

@danimtb danimtb merged commit c445625 into conan-io:master Feb 24, 2020
@SpaceIm SpaceIm deleted the add-lzfse-1.0 branch February 24, 2020 10:36
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.

5 participants