Bug 2019440 - usd-21.11 is available
Summary: usd-21.11 is available
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: usd
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Luya Tshimbalanga
QA Contact:
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2021-11-02 14:47 UTC by Upstream Release Monitoring
Modified: 2021-12-02 01:29 UTC (History)
3 users (show)

Fixed In Version: usd-21.11-4.fc36
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2021-11-28 18:39:20 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
Fixes the warning (but not the build) (1.21 KB, patch)
2021-11-06 20:30 UTC, Ben Beasley
no flags Details | Diff
ABI diff (350.42 KB, text/plain)
2021-11-13 20:27 UTC, Ben Beasley
no flags Details

Description Upstream Release Monitoring 2021-11-02 14:47:08 UTC
Latest upstream release: 21.11
Current version/release in rawhide: 21.08-17.fc36
URL: https://www.openusd.org/

Please consult the package updates policy before you issue an update to a stable branch: https://docs.fedoraproject.org/en-US/fesco/Updates_Policy/


More information about the service that created this bug can be found at: https://fedoraproject.org/wiki/Upstream_release_monitoring


Please keep in mind that with any upstream change, there may also be packaging changes that need to be made. Specifically, please remember that it is your responsibility to review the new version to ensure that the licensing is still correct and that no non-free or legally problematic items have been added upstream.


Based on the information from anitya: https://release-monitoring.org/project/197450/

Comment 1 Upstream Release Monitoring 2021-11-02 14:47:15 UTC
Skipping the scratch build because an SRPM could not be built: ['git', 'commit', '-a', '-m', 'Update to 21.11 (#2019440)'] returned 1: b'On branch rawhide\nYour branch is up to date with \'origin/rawhide\'.\n\nUntracked files:\n  (use "git add <file>..." to include in what will be committed)\n\tSRPMS/\n\nnothing added to commit but untracked files present (use "git add" to track)\n'

Comment 3 Luya Tshimbalanga 2021-11-06 01:40:40 UTC
Scratch build failed at the following lines:
--
/builddir/build/BUILD/USD-21.11/pxr/usd/sdf/path.h: In constructor 'pxrInternal_v0_21__pxrReserved__::SdfPath::SdfPath()':
/builddir/build/BUILD/USD-21.11/pxr/usd/sdf/path.h:309:15: warning: 'void* memset(void*, int, size_t)' clearing an object of type 'class pxrInternal_v0_21__pxrReserved__::SdfPath' with no trivial copy-assignment; use assignment or value-initialization instead [-Wclass-memaccess]
  309 |         memset(this, 0, sizeof(*this));
      |         ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
/builddir/build/BUILD/USD-21.11/pxr/usd/sdf/path.h:288:7: note: 'class pxrInternal_v0_21__pxrReserved__::SdfPath' declared here
  288 | class SdfPath : boost::totally_ordered<SdfPath>
      |       ^~~~~~~
cc1plus: some warnings being treated as errors
gmake[2]: *** [pxr/imaging/hd/CMakeFiles/hd.dir/build.make:1179: pxr/imaging/hd/CMakeFiles/hd.dir/dirtyList.cpp.o] Error 1
---

No idea about the meaning and unsure what to do.

Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=78392176
COPR build which includes SPEC file: https://copr.fedorainfracloud.org/coprs/luya/blender-egl/build/2932095/

@code could you check please?

Comment 4 Ben Beasley 2021-11-06 15:44:52 UTC
Sure, I’ll take a look at it as soon as I have a chance—probably in the next couple of days.

It *looks* like the kind of thing I could probably understand pretty quickly. The error message makes sense to me, but I’ll have to examine the context to know what to do about it.

Comment 5 Ben Beasley 2021-11-06 20:30:08 UTC
Created attachment 1840493 [details]
Fixes the warning (but not the build)

This patch fixes the warning by dropping the performance workaround for older compilers in the SdfPath default constructor, and instead simply giving SdfPath an explicitly-defaulted default constructor.

However, note that this warning was not treated as an error, and it is not the reason the build failed. That was:

> /builddir/build/BUILD/USD-21.11/pxr/imaging/hd/dirtyList.cpp: In member function 'void pxrInternal_v0_21__pxrReserved__::HdDirtyList::UpdateRenderTagsAndReprSelectors(const TfTokenVector&, const HdReprSelectorVector&)':
> /builddir/build/BUILD/USD-21.11/pxr/imaging/hd/dirtyList.cpp:193:38: error: format not a string literal and no format arguments [-Werror=format-security]
>   193 |                 TfDebug::Helper().Msg(ss.str().c_str());
>       |                 ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~

Several copies of the warning appeared after the error because the build is parallel, so other translation units were also being compiled—and producing warnings—when the error occurred. It’s common that the error is not the last diagnostic.

I’ll investigate the error next.

Comment 6 Ben Beasley 2021-11-06 20:43:12 UTC
While it’s true we no longer need to backport 8f9bb9563980b41e7695148b63bf09f7abd38a41.patch to update the bundled stb libraries to the latest upstream versions, there are still pending PR’s in the stb upstream to fix published CVE’s. These are already patched into the versions of the stb libraries in Fedora, so we should keep doing the trick from https://src.fedoraproject.org/rpms/usd/c/50dbcf7a3fcecb96aeb700c1e78c17c3c843ec70?branch=rawhide (unbundle then apply usd-specific patches from the usd tree) so that usd has fixes for those CVE’s too.

Comment 7 Ben Beasley 2021-11-07 02:33:45 UTC
Here are the lines around the error:

>             // Reset tracked repr set.
>             // XXX An alternative is to grow the tracked repr set similar to 
>             //     render tags (above). This will require the render index to
>             //     sync the tracked reprs rather than ones requested by the
>             //     tasks.
>             if (TfDebug::IsEnabled(HD_DIRTY_LIST)) {
>                 std::stringstream ss;
>                 ss << "Resetting tracked reprs in dirty list from "
>                    << _trackedReprs << " to " << reprs << "\n";
>                 TfDebug::Helper().Msg(ss.str().c_str());
>             }

For the definition of class TfDebug, see pxr/base/tf/debug.h. We find that TfDebug::Helper is a struct (same as a class except default public instead of default private visiblity) defined as follows:

>     struct Helper {
>         static TF_API void Msg(const std::string& msg);
>         static TF_API void Msg(const char* msg, ...) ARCH_PRINTF_FUNCTION(1,2);
>     };

The problem is that ss.str().c_str() is a “char const*”, so we are using the second overload of the TfDebug::Helper::Msg static method, which treats the string as a printf-style format string. This is an opportunity for havoc (https://capec.mitre.org/data/definitions/135.html), which is why it is marked with the ARCH_PRINTF_FUNCTION(1, 2) macro—which expands to __attribute__((format(printf, 1, 2))), telling GCC it is a printf-style formatting function, and triggering a warning if the format string is not a compile-time constant—and why Fedora’s default compiler flags asks GCC to treat this warning as an error (-Werror=format-security).

This *is* a bug that should be fixed upstream. The fix is simple: use the std::string copy of the std::stringstream contents directly in order to use the first overload:

>                 TfDebug::Helper().Msg(ss.str());

If we had a C string (const char*), we could either initialize a std::string from it to use the first overload, or provide a trivial format string "%s" for the second overload, like the overly-complicated:

>                 TfDebug::Helper().Msg("%s", ss.str().c_str());

I’ll file an upstream issue with a similar writeup, submit a PR, and link it here, then I’ll finally clear my NEEDINFO.

Comment 8 Ben Beasley 2021-11-07 15:51:41 UTC
Okay, here’s the upstream issue: https://github.com/PixarAnimationStudios/USD/issues/1675

…and the upstream PR: https://github.com/PixarAnimationStudios/USD/pull/1676

which you can use like:

> Patch123:       https://github.com/PixarAnimationStudios/USD/issues/1676.patch

That should be all you need to get a successful build.

----

Do note the new library name prefixes (third item under Build in the upstream changelog, https://github.com/PixarAnimationStudios/USD/blob/v21.11/CHANGELOG.md#change-log). This makes the monolithic library name “libusd_usd_ms.so” instead of “libusd_ms.so”. You could:

1. adjust %files to the new name, or
2. disable the new name with -DPXR_LIB_PREFIX="lib", or
3. stop requesting a monolithic build with PXR_BUILD_MONOLITHIC, and get sensibly-names smaller libraries (but if you do this, make sure it doesn’t break the shared object versioning patch, c.f. https://bugzilla.redhat.com/show_bug.cgi?id=1895567#c19)

----

Let me know if there’s something else I can do to help.

Comment 9 Luya Tshimbalanga 2021-11-13 17:37:29 UTC
Thank you for the patch and I confirm the built was successful with the suggested changes so I opted for option 1 to keep simple. Once upstream fixes versioning, we will revisit option #3. I am pushing the update for only Rawhide and Fedora 35 due to Imath requirement

Comment 10 Ben Beasley 2021-11-13 20:26:35 UTC
(In reply to Luya Tshimbalanga from comment #9)
> Thank you for the patch and I confirm the built was successful with the
> suggested changes so I opted for option 1 to keep simple. Once upstream
> fixes versioning, we will revisit option #3.

Thanks! Sounds good.

> I am pushing the update for only Rawhide and Fedora 35 due to Imath requirement

Careful: you’re still versioning the shared object downstream, so it’s (even more) your responsibility to check for ABI changes in updates.

I couldn’t check the latest build with fedabipkgdiff since the shared library was renamed, but you’ll at least need to rebuild all dependencies in a side tag (just blender, I think?) along with this update due to the renaming of the shared object alone. A manual ABI diff gave a huge list of changes, including deletions, which I’ll upload as an attachment. This means the downstream soversion should be bumped as well (https://docs.fedoraproject.org/en-US/packaging-guidelines/#_downstream_so_name_versioning).

Due to the library renaming alone, I would recommend not making an update for Fedora 35, as an ABI-incompatible update would conflict with https://docs.fedoraproject.org/en-US/fesco/Updates_Policy/#stable-releases.

Since upstream provides no ABI stability guarantees and this is C++, I would suggest treating all updates as likely ABI-incompatibile in the future.

Comment 11 Ben Beasley 2021-11-13 20:27:14 UTC
Created attachment 1841586 [details]
ABI diff

Comment 12 Ben Beasley 2021-11-13 20:32:01 UTC
Maybe the ABI situation isn’t what I thought, as the changes all seem to be in the pxrInternal namespace or in Boost implementation details. I’m not sure if that means it’s actually compatible in practice or not. Anyway, renaming the shared library is certainly an incompatible change.

Comment 13 Luya Tshimbalanga 2021-11-14 07:40:12 UTC
(In reply to Ben Beasley from comment #10)
> I couldn’t check the latest build with fedabipkgdiff since the shared
> library was renamed, but you’ll at least need to rebuild all dependencies in
> a side tag (just blender, I think?) along with this update due to the
> renaming of the shared object alone. A manual ABI diff gave a huge list of
> changes, including deletions, which I’ll upload as an attachment. This means
> the downstream soversion should be bumped as well
> (https://docs.fedoraproject.org/en-US/packaging-guidelines/
> #_downstream_so_name_versioning).

Blender is the only package that requires USD. Fortunately the downstream soversion seems working fine as testing on COPR 
https://copr.fedorainfracloud.org/coprs/luya/blender-egl/build/2955787/

I will use side tag for Blender.

> 
> Due to the library renaming alone, I would recommend not making an update
> for Fedora 35, as an ABI-incompatible update would conflict with
> https://docs.fedoraproject.org/en-US/fesco/Updates_Policy/#stable-releases.
> 
> Since upstream provides no ABI stability guarantees and this is C++, I would
> suggest treating all updates as likely ABI-incompatibile in the future.

Will do

Comment 14 Fedora Update System 2021-11-28 18:39:20 UTC
FEDORA-2021-56e7db285f has been pushed to the Fedora 36 stable repository.
If problem still persists, please make note of it in this bug report.

Comment 15 Fedora Update System 2021-11-28 18:39:55 UTC
FEDORA-2021-cbd7b11d7d has been submitted as an update to Fedora 35. https://bodhi.fedoraproject.org/updates/FEDORA-2021-cbd7b11d7d

Comment 16 Fedora Update System 2021-11-29 02:49:36 UTC
FEDORA-2021-cbd7b11d7d has been pushed to the Fedora 35 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf upgrade --enablerepo=updates-testing --advisory=FEDORA-2021-cbd7b11d7d`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2021-cbd7b11d7d

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 17 Fedora Update System 2021-12-01 14:02:32 UTC
FEDORA-2021-cbd7b11d7d has been submitted as an update to Fedora 35. https://bodhi.fedoraproject.org/updates/FEDORA-2021-cbd7b11d7d

Comment 18 Fedora Update System 2021-12-02 01:29:30 UTC
FEDORA-2021-cbd7b11d7d has been pushed to the Fedora 35 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf upgrade --enablerepo=updates-testing --advisory=FEDORA-2021-cbd7b11d7d`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2021-cbd7b11d7d

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.


Note You need to log in before you can comment on or make changes to this bug.