Skip to content

[GLUTEN-10774][VL][FOLLOWUP] Strip quotes for osName and osVersion#10850

Merged
PHILO-HE merged 1 commit intoapache:mainfrom
liujiayi771:os-release
Oct 9, 2025
Merged

[GLUTEN-10774][VL][FOLLOWUP] Strip quotes for osName and osVersion#10850
PHILO-HE merged 1 commit intoapache:mainfrom
liujiayi771:os-release

Conversation

@liujiayi771
Copy link
Contributor

@liujiayi771 liujiayi771 commented Oct 8, 2025

What changes are proposed in this pull request?

Follow-up PR for #10774

The content in /etc/os-release is probably in the format of key="value". Previously, when extracting with regular expressions, the double quotes were removed. Currently, using Properties.load does not remove the double quotes, causing some startsWith logic to return false.

How was this patch tested?

N/A

@github-actions github-actions bot added the VELOX label Oct 8, 2025
@liujiayi771
Copy link
Contributor Author

liujiayi771 commented Oct 8, 2025

@yaooqinn @zhztheplayer Could you help to review? The issue is that our OS /etc/os-release file contains values wrapped in double quotes (e.g., VERSION_ID="3 (OpenAnolis Edition)"). Currently, this line in Gluten's code returns false.

https://github.com/apache/incubator-gluten/blob/f057958545480e3aa9be80c867a4d4455bbed33f/backends-velox/src/main/scala/org/apache/gluten/spi/SharedLibraryLoaderCentos8.scala#L28

NAME="Alibaba Cloud Linux"
VERSION="3 (OpenAnolis Edition)"

@zhztheplayer
Copy link
Member

Similar issue is repeatable from my end with Ubuntu 24.04 (although we don't have a loader for it). CC @yaooqinn for more insights.

If we are making the change maybe we also want to strengthen the accepts implementation code in the loader implementations by turning osName.contains("Ubuntu") to osName.startsWith("Ubuntu") / osName.equals("Ubuntu") or so.

@liujiayi771
Copy link
Contributor Author

liujiayi771 commented Oct 8, 2025

I believe that checking if osName contains some string is sufficient for matching. However, for osVersion, we need more precise matching. Currently, startsWith seems appropriate since there might be minor version numbers (e.g., CentOS 8.x or ), or cases like alinux that may include additional parenthetical information (e.g., 3 (OpenAnolis Edition)).

We don't need to use equals for exact matching. In many cases, the system can still run normally on these operating systems even without an exact loader match.

@yaooqinn
Copy link
Member

yaooqinn commented Oct 9, 2025

It might be better to handle the returned values read from the system file on the caller side. Quotes might not be the only special character to deal with.

@PHILO-HE PHILO-HE changed the title [VL][10774][FOLLOWUP] strip quotes for osName and osVersion [VL][GLUTEN-10774][FOLLOWUP] Strip quotes for osName and osVersion Oct 9, 2025
@liujiayi771
Copy link
Contributor Author

liujiayi771 commented Oct 9, 2025

@yaooqinn We can first remove the double quotes as a quick fix to maintain consistency with previous results. I think the vast majority of operating systems will only have either key=value or key="value" formats.

https://github.com/apache/incubator-gluten/blob/50dd117dadb9fe3e9bba5c5002168e1294b2e2bb/backends-velox/src/main/scala/org/apache/gluten/utils/SharedLibraryLoader.scala#L68

@PHILO-HE PHILO-HE changed the title [VL][GLUTEN-10774][FOLLOWUP] Strip quotes for osName and osVersion [GLUTEN-10774][VL][FOLLOWUP] Strip quotes for osName and osVersion Oct 9, 2025
@github-actions
Copy link

github-actions bot commented Oct 9, 2025

#10774

Copy link
Member

@PHILO-HE PHILO-HE left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks.

@PHILO-HE PHILO-HE merged commit ceff106 into apache:main Oct 9, 2025
102 of 103 checks passed
@liujiayi771 liujiayi771 deleted the os-release branch October 9, 2025 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants