Skip to content

[open62541] Update to 1.3.15, fix android#42470

Merged
BillyONeal merged 3 commits intomicrosoft:masterfrom
dg0yt:open62451
Dec 4, 2024
Merged

[open62541] Update to 1.3.15, fix android#42470
BillyONeal merged 3 commits intomicrosoft:masterfrom
dg0yt:open62451

Conversation

@dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Dec 2, 2024

For #42080.

@dg0yt dg0yt mentioned this pull request Dec 2, 2024
7 tasks
@dg0yt dg0yt changed the title [open62541] Update, fix android [open62541] Update to 1.3.15, fix android Dec 2, 2024
@jimwang118 jimwang118 added the category:port-update The issue is with a library, which is requesting update new revision label Dec 2, 2024
@jimwang118
Copy link
Contributor

The latest upstream version is 1.4.8, please update to the latest version.

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 2, 2024

The latest upstream version is 1.4.8, please update to the latest version.

No, not in this PR.
1.3.15 isn't obsolete. It was released at the same time as 1.4.8 ("last week").
1.4.8 needs Android API level 24. vcpkg CI is at 21.

@Neumann-A
Copy link
Contributor

1.4.8 also doesn't work with qtocupa so....

@BillyONeal
Copy link
Member

Are these patches submitted upstream and/or can you explain how to check that they are correct?

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 3, 2024

These patches are trivial. The PR is cherry-picked from #42080 where it fixes downstream linking (qtopcua). Upstream has similar changes in 1.4.

There is no lib rt on Android.
(Upstream has AND NOT ANDROID_NDK_TOOLCHAIN_INCLUDED now, but that's not the right variable.)

There is no need to generally enable sanitizers in clang debug builds. In static builds, sanitizers impose transitive usage requirements.
The patch is just equivalent to if(0). (Upstream has UA_ENABLE_DEBUG_SANITIZER now.)

@jimwang118 jimwang118 added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Dec 3, 2024
@jimwang118
Copy link
Contributor

All feature test passed with x64-android triplet.

@BillyONeal
Copy link
Member

These patches are trivial. The PR is cherry-picked from #42080 where it fixes downstream linking (qtopcua).

Sure, I wasn't trying to claim that they were not trivial. Just because they're trivial doesn't mean I'm not taking responsibility for them when merged though, so I still want some understanding of what they're doing.

Upstream has similar changes in 1.4.
[...]
There is no lib rt on Android.
(Upstream has AND NOT ANDROID_NDK_TOOLCHAIN_INCLUDED now, but that's not the right variable.)

OK, I'm happy that this is morally equivalent to https://github.com/open62541/open62541/blob/642cf4b59d9ac99675f3ccdc33ef1314c21b454a/CMakeLists.txt#L451

Upstream has similar changes in 1.4.
[...]
There is no need to generally enable sanitizers in clang debug builds. In static builds, sanitizers impose transitive usage requirements.
The patch is just equivalent to if(0).

I looked at upstream before asking about this and I don't see it:

https://github.com/search?q=repo%3Aopen62541%2Fopen62541+UA_ENABLE_SANITIZER&type=code
https://github.com/search?q=repo%3Aopen62541%2Fopen62541+UA_ENABLE_SANTITIZER&type=code

(Upstream has UA_ENABLE_DEBUG_SANITIZER now.)

Yes, I see open62541/open62541@a820093 , but that's functionally always on in upstream, not off. Though looking now it looks like that stuff should come from the triplet. Unfortunately, patches don't contain all that context :(


# Debug
if(BUILD_TYPE_LOWER_CASE STREQUAL "debug" AND UNIX AND NOT UA_BUILD_OSS_FUZZ AND
+ UA_ENABLE_SANTITIZER AND
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
+ UA_ENABLE_SANTITIZER AND
+ UA_ENABLE_SANITIZER AND

?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@BillyONeal BillyONeal Dec 4, 2024

Choose a reason for hiding this comment

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

(To be clear I would have just merged this if not for this spelling oops but since a revision is necessary anyway may as well match upstream's name?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"patch does not apply"

Copy link
Member

Choose a reason for hiding this comment

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

😢

@BillyONeal BillyONeal marked this pull request as draft December 4, 2024 04:36
@dg0yt dg0yt marked this pull request as ready for review December 4, 2024 06:05
@BillyONeal BillyONeal merged commit 3d88aeb into microsoft:master Dec 4, 2024
@BillyONeal
Copy link
Member

Thanks for the update and for fixing Android!

@dg0yt dg0yt deleted the open62451 branch December 5, 2024 04:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:port-update The issue is with a library, which is requesting update new revision info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants