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

boost: Package Version Update -> 1.62.0 #3266

Closed
wants to merge 5 commits into from

Conversation

ClaymorePT
Copy link
Contributor

@ClaymorePT ClaymorePT commented Sep 30, 2016

Maintainer: @ClaymorePT
Compile tested: Broadcom BCM2708
Run tested: None

Description:
This package version update brings two new libraries:

  • Fiber 1
    • Framework for userland-threads/fibers, from Oliver Kowalke.
  • QVM 2
    • Boost QVM is a generic library for working with quaternions, vectors
      and matrices of static size with the emphasis on 2, 3 and
      4-dimensional operations needed in graphics, video games and
      simulation applications, from Emil Dotchevski.

More information about the 1.62.0 release (bug fixes, etc), can be found
here 3.

Signed-off-by: Carlos M. Ferreira carlosmf.pt@gmail.com

@ClaymorePT
Copy link
Contributor Author

@thess @diizzyy @hnyman
Comments?

Copy link
Member

@thess thess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just my first pass review - I haven't finishing build testing yet. Please wait for my followup coming in a bit.

Also, please do not keep deleting the PR to update it. Just fix the code, rebase/squash (amend message if necessary) and then push --force. Github will correctly update your PR.

include $(INCLUDE_DIR)/target.mk

PKG_NAME:=boost
PKG_VERSION:=1_61_0
PKG_VERSION:=1.62.0
Copy link
Member

@thess thess Sep 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add PKG_SOURCE_VERSION:=1_62_0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was following @diizzyy sugestion in #3265 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can have both PKG_VERSION:=1.62.0 and PKG_SOURCE_VERSION=1_62_0 and use them where appropriate. No need to repeat the 1_62_0 literal for every reference. Makes updating much easier.

PKG_RELEASE:=1

PKG_SOURCE:=$(PKG_NAME)_$(PKG_VERSION).tar.gz
PKG_SOURCE:=$(PKG_NAME)_1_62_0.tar.gz
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use PKG_SOURCE_VERSION here

HOST_BUILD_DIR:=$(BUILD_DIR_HOST)/$(PKG_NAME)_$(PKG_VERSION)
PKG_MD5SUM:=874805ba2e2ee415b1877ef3297bf8ad
PKG_BUILD_DIR:=$(BUILD_DIR)/$(PKG_NAME)_1_62_0
HOST_BUILD_DIR:=$(BUILD_DIR_HOST)/$(PKG_NAME)_1_62_0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use PKG_SOURCE_VERSION as above

PKG_MD5SUM:=874805ba2e2ee415b1877ef3297bf8ad
PKG_BUILD_DIR:=$(BUILD_DIR)/$(PKG_NAME)_1_62_0
HOST_BUILD_DIR:=$(BUILD_DIR_HOST)/$(PKG_NAME)_1_62_0
PKG_SHA256SUM:=440a59f8bc4023dbe6285c9998b0f7fa288468b889746b1ef00e8b36c559dce1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no PKG_SHA256SUM tag. Use PKG_MD5SUM. The download code will inspect this tag to determine the length of the checksum and handle it appropriately as MD5 or SHA256 - really.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the building system did not throw any error, I assumed it was ok.
I guess I was wrong. I will fix it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build system lets you define any private variable you want. There are no checks for use and PKG_MD5SUM is not mandatory.

@thess
Copy link
Member

thess commented Sep 30, 2016

Some general comments...

  • "Include all Boost libraries." is not available even when BUILD_NLS=y.
  • Many white space diffs should be cleaned-up (unless on purpose)
  • GCC compiler version check should be .GE. 5 (no automatic failure when GCC v7 arrives)

@ClaymorePT
Copy link
Contributor Author

ClaymorePT commented Oct 2, 2016

"Include all Boost libraries." is not available even when BUILD_NLS=y.

It also needs GCC v5 or GCC v6 (or at least, it was my intention).

GCC compiler version check should be .GE. 5 (no automatic failure when GCC v7 arrives)

I did not understand what you mean with this. Could you rephrase it, please?

@thess
Copy link
Member

thess commented Oct 2, 2016

You should make the GCC version reference as any version greater-than-or-equal v5. Your makefile will not need to be updated whenever there is a GCC v7 and your package stops building. However, arithmetic comparisons are not easily done in 'make'.

Maybe the best test would be NOT GCC v4. However, changing these tests is not that important.

"Include all Boost Libraries" option should either work or be eliminated. Currently it is broken.

@ClaymorePT
Copy link
Contributor Author

ClaymorePT commented Oct 4, 2016

@thess please review it before I rebase the two commits into one.

I was unable to devise a way to detect the GCC version in a proper way. Instead, I decided to deny the selection of Boost.Coroutine2 and Boost.Fiber for GCC 4.8 which does not support the standard C++14.

If you know a simple and clear way of making these two libs depending on the compiler version being greater or equal to 5.0, let me know.

@thess
Copy link
Member

thess commented Oct 4, 2016

@ClaymorePT - Was there a good reason to remove the inclusion of nls.mk ?
Macros like $(ICONV_DEPENDS), etc. are null without it.

@thess
Copy link
Member

thess commented Oct 4, 2016

Another problem - The boost_1_62_0.tar.gz package on the Sourceforge mirror downloads is not always the same file - Checksum mismatch and sizes are different. I'm not sure how we fix this problem - try to notify the Sourceforge maintainers perhaps.

@diizzyy
Copy link
Contributor

diizzyy commented Oct 4, 2016

Is there a reason why we don't use bz2 tarball as it would save about 15mbyte of download?

@thess
Copy link
Member

thess commented Oct 4, 2016

@diizzyy - Well, there is a 7z package too which saves even more. I haven't pulled them all to check for any consistency in the 1.62.0 packages from different mirrors. My inclination right now is to drop back to 1.61.0, fix the NLS build issue and wait for @ClaymorePT to get us a good 1.62.0 package. Not having MPD build at the current time is a problem I'd like fixed.

@ClaymorePT
Copy link
Contributor Author

ClaymorePT commented Oct 4, 2016

@thess the inclusion of nls.mk in the past, was because I wanted to use the ICONV_DEPENDS config option. But because I wrongly thought that nls was already included by default, I wrongly removed it.
This is one of the things it pisses me off with all the menuconfig system.
For example, for me, because I have BUILD_NLS active in the menuconfig and because iconv was already built, the menuconfig does not bother to provide (at least) with a warning about using a config option that is not declared... This is sometimes not obvious to me and I fail to detect these bugs.

Regarding the boost_1_62_0.tar.gz file, that should not happen. The Checksum value comes directly from the Boost webpage. I'm not calculating it, it's the Boost devs that provide them for security reasons. See here

@diizzyy It never crossed my mind.
Does the menuconfig system handles bz2 tarballs ? If it does, I have no problem in changing it.

@ClaymorePT
Copy link
Contributor Author

Btw, the NLS issue does not exist with the previous makefile version.

I removed it for the 1.62.0 version because, again wrongly, I thought it was already included by default somewhere during the build.

@diizzyy
Copy link
Contributor

diizzyy commented Oct 4, 2016

@thess There's no 7z support in the package system, it does handle tar.gz, tar.bz2 and tar.xz just fine out of the box.

@ClaymorePT it works fine, make sure to do a build test before doing a commit. Also, if possible use SHA256 hashes (still using the PKG_MD5SUM variable) instead of MD5 to ensure integrity.

@ClaymorePT
Copy link
Contributor Author

I'm currently testing a build.
I have modified the Makefile to use SHA256 and tar.bz2. I also have re-included the nls.mk.

@thess
Copy link
Member

thess commented Oct 4, 2016

With respect to checksums - Yes, one of the 3 I managed to download did indeed match that value. However, the default downloaded in my Eastern US build environment did not match!

@thess
Copy link
Member

thess commented Oct 4, 2016

@diizzyy - It was a joke

@ClaymorePT
Copy link
Contributor Author

@thess was there any change in the way how SF links are expanded in the menuconfig system?

It seems that its the @sf that is not expanding correctly and is pulling from the SF boost repos, a snapshot of boost, instead of the release version.

@ClaymorePT
Copy link
Contributor Author

ClaymorePT commented Oct 4, 2016

@thess I have solved the issues.
I had to stop using the SF variable. It seems that its expansion no longer evaluates to a correct url or it no longer works with Source Forge.
With a partial direct link, it seems to work correctly.

Upon your acceptance, I will rebase everything into one commit, so that you can accept the pull-request.

@thess
Copy link
Member

thess commented Oct 4, 2016

@sf expands to only https://downloads.sourceforge.net/. The URI being used is 301 redirected to the snapshots. The following should work for static boost versions:

PKG_NAME:=boost
PKG_VERSION:=1.61.0
PKG_SOURCE_VERSION=1_61_0
PKG_RELEASE:=1

PKG_SOURCE:=$(PKG_NAME)$(PKG_SOURCE_VERSION).tar.gz
PKG_SOURCE_URL:=@SF/project/boost/boost/$(PKG_VERSION)
PKG_BUILD_DIR:=$(BUILD_DIR)/$(PKG_NAME)
$(PKG_SOURCE_VERSION)
HOST_BUILD_DIR:=$(BUILD_DIR_HOST)/$(PKG_NAME)_$(PKG_SOURCE_VERSION)

You will need both PKG_VERSION and PKG_SOURCE_VERSION as I previously suggested.

@thess
Copy link
Member

thess commented Oct 4, 2016

@ClaymorePT - appologies for 75a74f4. I just wanted to get boost and friends building again while we sort out the 1.62.0 update.

@ClaymorePT
Copy link
Contributor Author

Well, your sugestion to solve the @sf issue does not work. :/
The expanded URL is being redirected to the snapshots folder, which aren't the release versions, and fails the checksum evaluation.

@ClaymorePT
Copy link
Contributor Author

ClaymorePT commented Oct 4, 2016

@thess That's ok. I understand the urgence of the necessity. Boost is a very important set of tools for serious C++ developers. It is also important for me and other research groups at IT

@ClaymorePT
Copy link
Contributor Author

ClaymorePT commented Oct 5, 2016

@diizzyy Yes, but the issue is with how the SF variable is being expanded.
This link will also and always give the release tarballs, with the extra of redirecting to the best mirror.
https://sourceforge.net/projects/boost/files/boost/1.62.0/boost_1_62_0.tar.bz2

@thess
Copy link
Member

thess commented Oct 5, 2016

Hey guys, there is nothing wrong with @sf - see my amended comment above (added /project). Thanks @diizzyy for catching that.

@ClaymorePT
Copy link
Contributor Author

ClaymorePT commented Oct 5, 2016

but the thing is, if I have
PKG_SOURCE_URL:=https://sourceforge.net/projects/boost/files/boost/$(PKG_VERSION)
it works, but if I have
PKG_SOURCE_URL:=@SF/project/boost/boost/$(PKG_VERSION)
it does not work...

If the SF expanded url is getting the snapshot version, for the case of Boost, then it is not getting the correct tarball. According to Rene from the Boost dev team, the snapshots aren't the release versions, and that is why the checksum was failing.
See here Rene's answer to my question at the boost mailing list.

@thess
Copy link
Member

thess commented Oct 5, 2016

OK - I guess we should just use what works for now and get this submission moving forward again. We can tweak it later for efficiency and clarity.

@ClaymorePT
Copy link
Contributor Author

@thess Rebase done. Please review it when you have the time.

@@ -320,6 +347,10 @@ TARGET_CFLAGS += \
$(if $(CONFIG_PACKAGE_boost-python3), -I$(STAGING_DIR)/usr/include/python3.5/) \
$(if $(CONFIG_SOFT_FLOAT),-DBOOST_NO_FENV_H) -fPIC

TARGET_CXXFLAGS += \
$(if $(or $(CONFIG_GCC_USE_VERSION_5), $(CONFIG_GCC_USE_VERSION_6)), -std=c++14, -std=c++11)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bug here. Please hold the merge of this commit.

It is not intended to be like this. It should use C++11 on GCC_VERSION_4_8 detection or C++14 otherwise.

Maintainer: @ClaymorePT
Compile tested: Broadcom BCM2708
Run tested: None

Description:
This package version update brings two new libraries:
- Fiber [1]
  - Framework for userland-threads/fibers, from Oliver Kowalke.
- QVM [2]
  - Boost QVM is a generic library for working with quaternions, vectors
    and matrices of static size with the emphasis on 2, 3 and
    4-dimensional operations needed in graphics, video games and
    simulation applications, from Emil Dotchevski.

More information about the 1.62.0 release (bug fixes, etc), can be found
  here [3].

[1]: http://www.boost.org/doc/libs/1_62_0/libs/fiber/
[2]: http://www.boost.org/doc/libs/1_62_0/libs/qvm/
[3]: http://www.boost.org/users/history/version_1_62_0.html

Signed-off-by: Carlos M. Ferreira <carlosmf.pt@gmail.com>
dependencies on pthread and librt are still necessary. Boost.Build will just include them
- dependencies updates
- libbz2 support added
@thess
Copy link
Member

thess commented Oct 7, 2016

In case you haven't noticed, this PR has too many whitespace errors and conflicts to be merged automatically. I think it is time to clean up this submission. Resolve the conflicts and fix the whitespace diffs. Squash, rebase and re-submit.

I don't know why we should have to review changes like:

- DEPENDS:=+libstdcpp +librt +libpthread
+ DEPENDS:=+libstdcpp +libpthread +librt

BTW - You will need libpthread and librt when building with something other than musl. With musl they are benign and don't require any further qualifications.

@ClaymorePT
Copy link
Contributor Author

ClaymorePT commented Oct 8, 2016

BTW - You will need libpthread and librt when building with something other than musl. With musl they are benign and don't require any further qualifications.

Ok. Got it. I will add it. I was trying to clean the Makefile in order to understand some issues.
This is still a work in progress.

Also, this is going to need more effort. From what I can tell, Boost.Fiber is being built due to the lack of C++11 features, more specifically, the atomic locks.
I have sent an e-mail to the Boost-users mailing list.

I'm still trying to understand what is causing this. I don't know if its because the '-march' flag is being passed with 'armv6' instead of 'armv6k' or not, for the Raspberry Pi B platform.

I will keep in touch.

@ClaymorePT
Copy link
Contributor Author

ClaymorePT commented Oct 9, 2016

@thess I have decided to close this pull request until I figure out what is going wrong when compilling Boost.Fiber. If necessary, I will issue another pull-request but without support for Boost.Fiber, by marking it as BROKEN.
I will also address the issues you pointed out.

Also, the USE_MUSL symbol is not working as intended.
For example, for boost.locale.posix=$(if $(USE_MUSL),on,off) always results in boost.locale.posix=off even if musl is used.

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

Successfully merging this pull request may close these issues.

None yet

3 participants