Skip to content

Delete old boost ifdeffery #1087

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

vincele
Copy link
Contributor

@vincele vincele commented Apr 9, 2025

Code Changes:

  • Have the PR Validation Tests been run? compiles & runs on linux
  • This is a documentation change only

Issues:

  • not tested on any other platform

Purpose:

  • What is this pull request trying to do? remove old unused code
  • What release is this for? next
  • Is there a project or milestone we should apply this to? next

Closes: #797

Comment on lines -124 to -145
inline PyObject * to_python( Vector vec )
{
return to_python( boost::python::tuple( (double) vec.i, (double) vec.j, (double) vec.k ) );
}
inline PyObject * to_python( QVector vec )
{
return to_python( boost::python::tuple( (double) vec.i, (double) vec.j, (double) vec.k ) );
}

inline Vector from_python( PyObject *p, boost::python::type< Vector >)
{
Vector vec( 0, 0, 0 );
PyArg_ParseTuple( p, "fff", &vec.i, &vec.j, &vec.k );
return vec;
}
inline QVector from_python( PyObject *p, boost::python::type< QVector >)
{
QVector vec( 0, 0, 0 );
PyArg_ParseTuple( p, "ddd", &vec.i, &vec.j, &vec.k );
return vec;
}
#endif //BOOST_VERSION != 102800
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there was a reason why this code was here. I'd rather not remove it without careful consideration.

Over time, I've gotten the impression that there were two separate embedded Python environments: the standard one, using Boost Python 1.73 or 1.87 or a similar recent version, and then a weird, internal-only Python environment, pinned at Boost Python 1.28. Some of these same source files may be reused between both.

I could be mistaken, or not quite correct. Either way, I'd like confirmation before we go deleting any code that might actually still be relevant.

Copy link
Member

Choose a reason for hiding this comment

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

When we picked up VS in 2019 it was using Boost 1.53; the Mac build seemed to support 1.29 as well.
(Found this commit from @klaussfreire - https://github.com/stephengtuggy/Vega-Strike-Engine-Source/tree/36d551e2b812ee038f745b90a451abb6df667bdb - way back in Oct 2019.... wow we've come a long way since then...) There was also a copy of the boost libraries embedded in the source that we quickly removed in favor of just using the system versions.

I think it's safe to say we won't be supporting those older versions any more.

As a general rule, we need to look at what is available on the various platforms we're supporting, which is probably now driven by the Ubuntu LTS releases (22.04, 24.04), Debian (bookworm), or Rocky (9.5) more than anything else. Mac and Windows its mostly a matter of what we can get from Homebrew (Mac) and Nuget/Chocolatey (Windows) in our CI platforms.

Comment on lines -94 to -107
TO_PYTHON_SMART_POINTER( UnitWrapper );
TO_PYTHON_SMART_POINTER( Cargo );

PYTHON_INIT_GLOBALS( Unit, UnitWrapper )
BOOST_PYTHON_BEGIN_CONVERSION_NAMESPACE
inline PyObject*to_python( Unit*un )
{
return to_python( UnitWrapper( un ) );
}
inline Unit * from_python( PyObject *p, boost::python::type< Unit* >)
{
UnitWrapper uw = ( from_python( p, boost::python::type< UnitWrapper& > () ) );
return uw.GetUnit();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is another block that I'm thinking maybe we should keep.

Copy link
Member

@BenjamenMeyer BenjamenMeyer left a comment

Choose a reason for hiding this comment

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

Let's continue the conversation with @stephengtuggy regarding some of the code-blocks and resolve those before merging.

But from my perspective it's good to go.

Copy link
Contributor

@stephengtuggy stephengtuggy left a comment

Choose a reason for hiding this comment

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

Let's hold until we've finished discussing these changes. Thanks

@BenjamenMeyer BenjamenMeyer added this to the 0.10.x milestone Apr 10, 2025
@stephengtuggy stephengtuggy moved this to In progress in 0.10.x Release Apr 13, 2025
@royfalk
Copy link
Contributor

royfalk commented Jun 22, 2025

@vincele are you still in the loop or should I take over?
@stephengtuggy how do we move on here? If we remove the two sensitive sections, are you happy with the rest?
Or is more needed?

@vincele
Copy link
Contributor Author

vincele commented Jun 22, 2025

@royfalk you can go ahead and take over

@stephengtuggy
Copy link
Contributor

@royfalk No, neither of the sections noted should be removed, in my opinion. The best way to move forward would probably be to get some input from one or more members of the original development team -- find out which Python versions they were using, and how, and why.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

Revisit whether to keep code that references Boost version 1.28
4 participants