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

compute/detail/sha1.hpp fails to compile with boost 1.86 #889

Open
bredelings opened this issue Aug 21, 2024 · 20 comments
Open

compute/detail/sha1.hpp fails to compile with boost 1.86 #889

bredelings opened this issue Aug 21, 2024 · 20 comments

Comments

@bredelings
Copy link

When compiling files that include <boost/compute/detail/sha1.hpp>, I see the following error:

/home/bredelings/Work/installed-boost-1.86.0/include/boost/compute/detail/sha1.hpp: In member function ‘boost::compute::detail::sha1::operator std::string()’:
/home/bredelings/Work/installed-boost-1.86.0/include/boost/compute/detail/sha1.hpp:41:26: error: cannot convert ‘unsigned int [5]’ to ‘unsigned char (&)[20]’
   41 |             h.get_digest(digest);
      |                          ^~~~~~
      |                          |
      |                          unsigned int [5]
In file included from /home/bredelings/Work/installed-boost-1.86.0/include/boost/compute/detail/sha1.hpp:18:
/home/bredelings/Work/installed-boost-1.86.0/include/boost/uuid/detail/sha1.hpp:179:43: note:   initializing argument 1 of ‘void boost::uuids::detail::sha1::get_digest(unsigned char (&)[20])’
  179 | inline void sha1::get_digest(digest_type& digest)
      |                              ~~~~~~~~~~~~~^~~~~~

This is because boost::uuids::detail::sha1::digest_type has changed from typedef unsigned int(digest_type)[5]; to typedef unsigned char digest_type[ 20 ];.

@nega0
Copy link

nega0 commented Aug 21, 2024

You'll probably have to cross post this to boostorg/uuid and the boost developers mailing list in order to get some movement on it.

@bredelings
Copy link
Author

Fixed by #888

@bredelings
Copy link
Author

Also fixed (perhaps in a better way) by #887

@nega0
Copy link

nega0 commented Aug 22, 2024

yeah, it's nice that it's in PRs, but it doesn't matter until it's in a proper point release. a new volunteer stepped up this summer to maintain boost::compute, but until they're fully on-boarded and these fixes make it into a proper release boost::compute v1.86 is dead in the water

@kylelutz
Copy link
Collaborator

Thanks for the report! #887 has been merged to develop, and will make it into the next release.

@bredelings
Copy link
Author

That is great. Can we have a point release to fix this, since it breaks existing software.

@glenfe
Copy link
Member

glenfe commented Aug 22, 2024

@bredelings would official patches on https://www.boost.org/patches/ against 1.86 for both UUID and Compute suffice?

@bredelings
Copy link
Author

Do we need to patch UUID too? It seemed like patching Compute would be sufficient.

Hmm. I guess what I would hope is that

  1. official packages from homebrew and Linux distributions like Debian would include the patch.
  2. there is a tarball representing the patched version that I can plug into existing scripts that compile boost during CI.
  3. there is a version number associated with the patched source.

A new point release would accomplish all of these goals, but maybe only (1) is really essential. Without that all the CI infrastructure using SHA1 from Compute needs to be updated to compile boost 1.85 from scratch. (2) is helpful so that CI infrastructure that already compiles boost can use the same code for 1.86 as for 1.85 -- download a tarball and compile.

I don't know hard hard spinning a new release is -- for my projects it is mostly automated. I see on the mailing lists that you all have a lot going on right now.

@bredelings
Copy link
Author

And obviously a test would be nice, to make sure this doesn't happen again. But Rome wasn't built in a day...

@bredelings
Copy link
Author

Also, I tried to join the mailing list yesterday, but haven't been approved yet :-P I see a thread of discussion about this has already been started though.

@pdimov
Copy link
Member

pdimov commented Aug 22, 2024

And obviously a test would be nice

The test is here: #892

@pdimov
Copy link
Member

pdimov commented Aug 22, 2024

  1. official packages from homebrew and Linux distributions like Debian would include the patch.

They generally do as they maintain a collection of patches anyway. But the issue needs to be reported to them.

@nega0
Copy link

nega0 commented Aug 22, 2024

@glenfe A point release would be preferred over patch files.

  1. Boost's patches aren't checksum'd
  2. https://boost.org/patches/ isn't referenced from anywhere else on the website
  3. Patches are only listed on individual release pages, à la https://www.boost.org/users/history/version_1_85_0.html, and then located inconsistently between versions
  4. Patches aren't even co-located with releases
  5. Patches are not considered "News". They are not on the front page, unlike releases. They are not on the News page, unlike releases.

In other words, patch discovery is terrible. One has to be "in the know" already. Release discovery is easy. One just has to visit https://boost.org.

@glenfe
Copy link
Member

glenfe commented Aug 23, 2024

@nega0 @bredelings We typically do a point release only for critical bugs, or if several libraries are affected and do not build. The release managers discussed this case and voted in favor of a patch.

But you make a good point about patch discovery being bad. We will correct that on https://boost.org and if the https://boost.io layout is adopted, we will make sure that patches are prominently featured there too.

In the meantime, can the relevant commits be merged to master?

@bredelings
Copy link
Author

I see that Debian/Ubuntu have not included the patch for 1.83, which is a bit worrisome. I guess this is the result of @nega0's points (4) and (5) above.

@glenfe, what do you think about posting @nega0's message to the boost list?

@nega0
Copy link

nega0 commented Aug 23, 2024

@nega0 @bredelings We typically do a point release only for critical bugs, or if several libraries are affected and do not build. The release managers discussed this case and voted in favor of a patch.

That's disappointing.

But you make a good point about patch discovery being bad. We will correct that on https://boost.org and if the https://boost.io layout is adopted, we will make sure that patches are prominently featured there too.

That's great. Speaking of the website, is the patch vs release decision making process documented there? If not, can it be? A paragraph or two at https://www.boost.org/patches/ would be a good location once its discoverability has been elevated

@glenfe, what do you think about posting @nega0's message to the boost list?

You're welcome to... I'm subscribed but can't seem to post. That's a "me" issue. Once I have that worked out I can jump into the discussion there.

@glenfe
Copy link
Member

glenfe commented Aug 24, 2024

@kylelutz After you merge the relevant commits to master, I will create upload a patch against 1.86 to the website and notify everyone.

@nega0 Yes, please post to the Boost developers list. If the sentiment is prominently echoed, it might justify the effort to go beyond a patch.

@thebrandre
Copy link

Just out of interest: will there be a an official patch on the website as suggested by the conversation in the mailing list Boost.Compute broken in 1.86.0 because of UUID changes, 1.86.1??

This would be nice to know! So I can simply wait for that before upgrading boost to 1.86.

@bredelings
Copy link
Author

bredelings commented Sep 29, 2024

My impression is that there is an official patch. However, it has been put in a secret place where nobody can find it.

@glenfe
Copy link
Member

glenfe commented Sep 30, 2024

The change in question still hasn't been merged to master, so I have to assume the maintainer of Compute (@kylelutz) does not desire an official patch on the Boost website.

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

No branches or pull requests

6 participants