0ad/0ad
0ad/0ad
11
9
Fork 21
Code Issues 1.5k Pull Requests 36 Actions Projects Releases Wiki Activity

Use upstream and allow system cxxtest #7056

Open
sera wants to merge 4 commits from sera/0ad:use-upstream-cxxtest into main
pull from: sera/0ad:use-upstream-cxxtest
merge into: 0ad:main
Conversation 14 Commits 4 Files Changed 14 +56 -32
sera commented 2024-09-18 19:59:58 +02:00
Member
Copy Link

This makes tests compatible with upstream cxxtest, replaces the custom version and allows system provided cxxtest to be used where available.

Closes: #5792

This makes tests compatible with upstream cxxtest, replaces the custom version and allows system provided cxxtest to be used where available. Closes: #5792
πŸ’― 1
sera added 3 commits 2024-09-18 20:00:02 +02:00 
To disable tests we carry a patch which allows disabling test by
appending DISABLED to the test function name. Instead just do as
upstream says and prepend the test function so it won't be recognized as
a test any longer.

The docs suggest to prepend x but anything will do. Continue to use
DISABLED_ but as prefix which is actually already in use in one case.

Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
Update source build.sh to use upstream tarball and drop any custom
patches at the same time.

Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
Allow system cxxtest
Some checks failed
checkrefs / checkrefs (pull_request) Successful in 1m11s
Details
pre-commit / build (pull_request) Successful in 1m29s
Details
0ad-windows/pipeline/pr-main This commit looks good
Details
0ad-linux/pipeline/pr-main There was a failure building this commit
Details
0ad-freebsd/pipeline/pr-main This commit is unstable
Details
0ad-macos/pipeline/pr-main This commit is unstable
Details
f3bdc2c7ce 
Add option to build-source-libs and premake to uses system provided
cxxtest.

Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
sera requested review from Itms 2024-09-18 20:00:02 +02:00
sera requested review from Stan 2024-09-18 20:00:02 +02:00
sera requested review from vladislavbelov 2024-09-18 20:00:02 +02:00
sera requested review from wraitii 2024-09-18 20:00:02 +02:00
sera commented 2024-09-18 20:09:43 +02:00
Author
Member
Copy Link

This is sort of a continuation of https://code.wildfiregames.com/D3119

This is sort of a continuation of https://code.wildfiregames.com/D3119
sera commented 2024-09-18 20:53:50 +02:00
Author
Member
Copy Link

One solution and probably the preferred one to get past the Linux build failure is to install the python-is-python3 package on the CI.

One solution and probably the preferred one to get past the Linux build failure is to install the python-is-python3 package on the CI.
Stan commented 2024-09-18 21:28:38 +02:00
Owner
Copy Link

You could also patch it like the other libs since it's vendored.

You could also patch it like the other libs since it's vendored.
sera commented 2024-09-18 22:24:11 +02:00
Author
Member
Copy Link

For completeness, invoking as python3 cxxtestgen or installing cxxtest on the CI and using --with-system-cxxtest would work as well.

Still, in light that it's fair to assume python is python3 this days and given upstream never changed it, installing the package python-is-python3 on the CI gets my vote. I prefer to only backport upstream patches instead of diverging unless we really don't have an alternative.

For completeness, invoking as python3 cxxtestgen or installing cxxtest on the CI and using --with-system-cxxtest would work as well. Still, in light that it's fair to assume python is python3 this days and given upstream never changed it, installing the package python-is-python3 on the CI gets my vote. I prefer to only backport upstream patches instead of diverging unless we really don't have an alternative.
Stan commented 2024-09-18 22:35:53 +02:00
Owner
Copy Link

I'm not sure but I think the CI now picks up docker changes?

I'm not sure but I think the CI now picks up docker changes?
πŸ”₯ 1
sera added 1 commit 2024-09-19 20:34:38 +02:00
Add python-is-python3 to Debian CI
Some checks failed
0ad-linux/pipeline/pr-main There was a failure building this commit
Details
checkrefs / checkrefs (pull_request) Failing after 1m24s
Details
pre-commit / build (pull_request) Successful in 1m30s
Details
0ad-windows/pipeline/pr-main This commit looks good
Details
0ad-freebsd/pipeline/pr-main This commit is unstable
Details
0ad-macos/pipeline/pr-main This commit is unstable
Details
c3b085e32d 
For the sake of vendored cxxtest. Distributions are advised to use
system cxxtest anyways.

From the package description:
This is a convenience package which ships a symlink to point the
/usr/bin/python interpreter at the current default python3. It may
improve compatibility with other modern systems, whilst breaking some
obsolete or 3rd-party software.

Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
Itms reviewed 2024-09-20 08:08:11 +02:00
Itms left a comment
Owner
Copy Link

Very nice patch and demonstration of the process for unbundling svn sources! πŸ’―

Very nice patch and demonstration of the process for unbundling svn sources! πŸ’―
build/jenkins/dockerfiles/buster-base.Dockerfile Outdated
@ -31,6 +31,7 @@ RUN apt-get -qqy update && apt-get install -qqy --no-install-recommends \
make \
m4 \
patch \
python-is-python3 \
Itms commented 2024-09-20 08:06:30 +02:00
Owner
Copy Link

This package only exists in bullseye and later. I suggest you create manually a symlink later in the dockerfile.

This package only exists in [`bullseye` and later](https://packages.debian.org/bullseye/python-is-python3). I suggest you create manually a symlink later in the dockerfile.
πŸ‘ 1
sera marked this conversation as resolved
libraries/source/cxxtest-4.4/build.sh Outdated
@ -5,2 +5,3 @@
LIB_VERSION=28209
PV=4.4
LIB_VERSION=${PV}
Itms commented 2024-09-20 08:03:23 +02:00
Owner
Copy Link

What's the rationale behind having both env vars?

What's the rationale behind having both env vars?
sera commented 2024-09-20 08:14:40 +02:00
Author
Member
Copy Link

LIB_VERSION could be changed to LIB_VERSION=${PV}+wfg1 when adding a patch later without upstream version bump, maybe should have that suffix always and start with wfg0 as for demonstration purpose.

LIB_VERSION could be changed to LIB_VERSION=${PV}+wfg1 when adding a patch later without upstream version bump, maybe should have that suffix always and start with wfg0 as for demonstration purpose.
sera marked this conversation as resolved
libraries/source/cxxtest-4.4/build.sh Outdated
@ -16,3 +18,2 @@
# unpack
rm -Rf cxxtest-4.4-build
cp -R cxxtest-4.4-svn cxxtest-4.4-build
rm -Rf cxxtest-4.4
Itms commented 2024-09-20 08:03:55 +02:00
Owner
Copy Link

Shouldn't you use the envvar for the directory name here?

Shouldn't you use the envvar for the directory name here?
sera commented 2024-09-20 08:15:11 +02:00
Author
Member
Copy Link

good catch

good catch
sera marked this conversation as resolved
libraries/source/cxxtest-4.4/build.sh Outdated
@ -23,3 +25,3 @@
# install
rm -Rf bin cxxtest python
cp -R cxxtest-4.4-build/bin cxxtest-4.4-build/cxxtest cxxtest-4.4-build/python .
cp -R cxxtest-4.4/bin cxxtest-4.4/cxxtest cxxtest-4.4/python .
Itms commented 2024-09-20 08:04:03 +02:00
Owner
Copy Link

and here?

and here?
πŸ‘ 1
sera marked this conversation as resolved
sera force-pushed use-upstream-cxxtest from c3b085e32d to aa0ad39fb5 2024-09-20 08:50:54 +02:00 Compare
Stan reviewed 2024-09-20 09:45:37 +02:00
libraries/source/cxxtest-4.4/build.sh Outdated
@ -7,3 +8,2 @@
if [ -e .already-built ] && [ "$(cat .already-built)" = "${LIB_VERSION}" ]; then
echo "cxxtest-4.4 is already up to date."
if [ -e .already-built ] && [ "$(cat .already-built || true)" = "${LIB_VERSION}" ]; then
Stan commented 2024-09-20 09:44:55 +02:00
Owner
Copy Link

Don't forget this.

Don't forget this.
sera commented 2024-09-20 10:10:31 +02:00
Author
Member
Copy Link

I don't see anything wrong with this line.

The .already-built shenanigans are the sole reason for ${LIB_VERSION}

I don't see anything wrong with this line. The .already-built shenanigans are the sole reason for ${LIB_VERSION}
Stan commented 2024-09-20 18:59:51 +02:00
Owner
Copy Link

|| true always true ?

|| true always true ?
sera commented 2024-09-20 19:45:49 +02:00
Author
Member
Copy Link

This only suppress the return value of cat, not what it writes to stdout. This is needed because of posix shell, set -e and ultimately shellcheck, because it insist that the cat command may fail.

This only suppress the return value of cat, not what it writes to stdout. This is needed because of posix shell, set -e and ultimately shellcheck, because it insist that the cat command may fail.
sera commented 2024-09-20 19:46:41 +02:00
Author
Member
Copy Link

from the man page:

true - do nothing, successfully

from the man page: true - do nothing, successfully
sera commented 2024-09-20 19:52:11 +02:00
Author
Member
Copy Link

https://www.shellcheck.net/wiki/SC2312

https://www.shellcheck.net/wiki/SC2312
sera marked this conversation as resolved
sera force-pushed use-upstream-cxxtest from aa0ad39fb5 to 5c981da29e 2024-09-24 19:41:27 +02:00 Compare
vladislavbelov commented 2024-10-02 20:23:01 +02:00
Member
Copy Link

I think fixes of includes might be done in a separate patch.

I think fixes of includes might be done in a separate patch.
sera commented 2024-10-02 20:33:34 +02:00
Author
Member
Copy Link

I think fixes of includes might be done in a separate patch.

It's a separate patch/commit and I intend to leave it as such, or do you mean separate pr instead?

> I think fixes of includes might be done in a separate patch. It's a separate patch/commit and I intend to leave it as such, or do you mean separate pr instead?
vladislavbelov commented 2024-10-02 20:37:19 +02:00
Member
Copy Link

It's a separate patch/commit and I intend to leave it as such, or do you mean separate pr instead?

Yeah, I meant a separate PR.

> It's a separate patch/commit and I intend to leave it as such, or do you mean separate pr instead? Yeah, I meant a separate PR.
sera commented 2024-10-02 20:42:03 +02:00
Author
Member
Copy Link

Yeah, I meant a separate PR.
Will do.

@Itms , the pipeline update doesn't seem reliable. Works on first attempted but when it's already in cache it seems to not load the right image somehow. Do you want to cherry-pick the Dockerfile update patch and if needed manually clear cache?

> Yeah, I meant a separate PR. Will do. @Itms , the pipeline update doesn't seem reliable. Works on first attempted but when it's already in cache it seems to not load the right image somehow. Do you want to cherry-pick the Dockerfile update patch and if needed manually clear cache?
Itms commented 2024-10-06 21:48:55 +02:00
Owner
Copy Link

@Itms , the pipeline update doesn't seem reliable. Works on first attempted but when it's already in cache it seems to not load the right image somehow. Do you want to cherry-pick the Dockerfile update patch and if needed manually clear cache?

I was very puzzled as nothing seemed to indicate why Docker wouldn't use the correct buster-base image. The only hypothesis I can offer is that the pipeline might have run in parallel with another PR and that the "Setup" stage for another run happened between the "Setup" and "Linux Build" of the build #4. Wanting to test that hypothesis, I restarted a run, this time I could see that nothing was run between the steps, and it worked. πŸ€·β€β™‚οΈ

> @Itms , the pipeline update doesn't seem reliable. Works on first attempted but when it's already in cache it seems to not load the right image somehow. Do you want to cherry-pick the Dockerfile update patch and if needed manually clear cache? I was very puzzled as nothing seemed to indicate why Docker wouldn't use the correct buster-base image. The only hypothesis I can offer is that the pipeline might have run in parallel with another PR and that the "Setup" stage for another run happened between the "Setup" and "Linux Build" of the build #4. Wanting to test that hypothesis, I restarted a run, this time I could see that nothing was run between the steps, and it worked. πŸ€·β€β™‚οΈ
sera force-pushed use-upstream-cxxtest from 5c981da29e to 33d734c717 2024-10-07 08:27:39 +02:00 Compare
sera force-pushed use-upstream-cxxtest from 33d734c717 to 6f6ed63b11 2024-10-08 07:02:08 +02:00 Compare
sera commented 2024-10-08 07:03:56 +02:00
Author
Member
Copy Link

Replaces /usr/bin/cxxtestgen with a search in PATH

Replaces /usr/bin/cxxtestgen with a search in PATH
Stan reviewed 2024-10-08 08:16:37 +02:00
build/premake/premake5.lua
@ -1404,1 +1405,3 @@
cxxtest.setpath(rootdir.."/libraries/source/cxxtest-4.4/bin/cxxtestgen")
if _OPTIONS["with-system-cxxtest"] then
local handle = io.popen("command -v cxxtestgen")
local cxxtestgen = handle:read("*a"):gsub("\n", " ")
Stan commented 2024-10-08 08:16:37 +02:00
Owner
Copy Link

Does this work if you have multiple installs?

Does this work if you have multiple installs?
sera commented 2024-10-08 09:50:49 +02:00
Author
Member
Copy Link

We also just assume that the headers/sources are in system includes, so this gives the user more freedom to make things work, arguably this is now more correct, in practice I don't expect this to make a difference. Sure It's not impossible to artificially construct a case where things can go wrong, hardcoding the path doesn't make that any harder tho.

We also just assume that the headers/sources are in system includes, so this gives the user more freedom to make things work, arguably this is now more correct, in practice I don't expect this to make a difference. Sure It's not impossible to artificially construct a case where things can go wrong, hardcoding the path doesn't make that any harder tho.
Stan commented 2024-10-08 12:34:29 +02:00
Owner
Copy Link

I meant can command -v cxxtestgen return

/usr/bin/cxxtestgen
/usr/local/bin/cxxtestgen
I meant can `command -v cxxtestgen` return ``` /usr/bin/cxxtestgen /usr/local/bin/cxxtestgen ```
sera commented 2024-10-08 14:06:05 +02:00
Author
Member
Copy Link

You mean if a single invocation can return multiple results? Then no, the \n needs stripping so that the block behaves sort of like the shell command substitution.

You mean if a single invocation can return multiple results? Then no, the \n needs stripping so that the block behaves sort of like the shell command substitution.
Stan commented 2024-10-08 16:06:12 +02:00
Owner
Copy Link

Okay. Would have been kinda annoying for it to call two programs.

Okay. Would have been kinda annoying for it to call two programs.
All checks were successful
checkrefs / checkrefs (pull_request) Successful in 56s
Required
Details
pre-commit / build (pull_request) Successful in 1m10s
Required
Details
0ad-windows/pipeline/pr-main This commit looks good
Required
Details
0ad-freebsd/pipeline/pr-main This commit looks good
Required
Details
0ad-macos/pipeline/pr-main This commit looks good
Required
Details
0ad-linux/pipeline/pr-main This commit looks good
Required
Details
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u use-upstream-cxxtest:sera-use-upstream-cxxtest
git checkout sera-use-upstream-cxxtest
Sign in to join this conversation.
Reviewers
No reviewers
Itms
Stan
vladislavbelov
wraitii
Labels
Clear labels   
Closed
Duplicate

  
Closed
Fixed

  
Closed
Invalid

  
Closed
Needs info

  
Closed
Won't fix

  
Closed
Works for me

  
Difficulty
Hard

  
Difficulty
Medium

  
Difficulty
Simple

  
Needed for Beta

   
Needs Design Input

   
Needs Info

   
Pathfinding
  
Priority
1: Release Blocker

  
Priority
2: Must Have

  
Priority
3: Should Have

  
Priority
4: Nice To Have

  
Priority
5: If Time Permits

  
Regression
  
Theme
AI

  
Theme
Art & Animation

  
Theme
Atlas editor

  
Theme
Build & Packages

  
Theme
Core engine

  
Theme
Documentation

  
Theme
Internationalization & Localization

  
Theme
Maps

  
Theme
Multiplayer Lobby

  
Theme
Music & Sound FX

  
Theme
Network

  
Theme
Non-game systems

  
Theme
Simulation

  
Theme
UI & Simulation

  
Theme
UI – Game setup

  
Theme
UI – In-game

  
Theme
UI – Miscellaneous

  
Theme
Website & Forum

  
Type
Defect

  
Type
Enhancement

  
Type
Task

No Label
Closed
Duplicate
Closed
Fixed
Closed
Invalid
Closed
Needs info
Closed
Won't fix
Closed
Works for me
Difficulty
Hard
Difficulty
Medium
Difficulty
Simple
Needed for Beta
Needs Design Input
Needs Info
Pathfinding
Priority
1: Release Blocker
Priority
2: Must Have
Priority
3: Should Have
Priority
4: Nice To Have
Priority
5: If Time Permits
Regression
Theme
AI
Theme
Art & Animation
Theme
Atlas editor
Theme
Build & Packages
Theme
Core engine
Theme
Documentation
Theme
Internationalization & Localization
Theme
Maps
Theme
Multiplayer Lobby
Theme
Music & Sound FX
Theme
Network
Theme
Non-game systems
Theme
Simulation
Theme
UI & Simulation
Theme
UI – Game setup
Theme
UI – In-game
Theme
UI – Miscellaneous
Theme
Website & Forum
Type
Defect
Type
Enhancement
Type
Task
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: 0ad/0ad#7056
No description provided.
Delete Branch "sera/0ad:use-upstream-cxxtest"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?