Skip to content
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

On master, drop Ubuntu 16.04 "Xenial" LTS and Debian 9 "stretch" #536

Merged

Conversation

stephengtuggy
Copy link
Contributor

@stephengtuggy stephengtuggy commented Aug 30, 2021

Thank you for submitting a pull request and becoming a contributor to the Vega Strike Core Engine.

Please answer the following:

Code Changes:

Issues:

Purpose:

  • What is this pull request trying to do? Drop support for Xenial and stretch on master, to pave the way for CMake v3.11 and gtest unit tests
  • What release is this for? 0.9.x/1.0.x
  • Is there a project or milestone we should apply this to? 0.9.x/1.0.x

@stephengtuggy stephengtuggy added this to the 0.9.x milestone Aug 30, 2021
@stephengtuggy stephengtuggy requested review from BenjamenMeyer and a team August 30, 2021 23:23
@stephengtuggy stephengtuggy self-assigned this Aug 30, 2021
@stephengtuggy stephengtuggy added this to In progress in 0.9.x Release Aug 30, 2021
@stephengtuggy
Copy link
Contributor Author

Should I drop Xenial and stretch from the build scripts and from CMakeLists.txt in the same PR? I suppose I should.

@stephengtuggy stephengtuggy marked this pull request as draft August 31, 2021 02:42
@stephengtuggy
Copy link
Contributor Author

Thanks, @nabaco . Unfortunately, I had added 4 more commits on my local machine last night, and another one this morning. Hopefully it's not too much trouble to review those too?

@stephengtuggy stephengtuggy marked this pull request as ready for review August 31, 2021 16:20
@stephengtuggy stephengtuggy changed the title GitHub Actions: Drop Ubuntu 16.04 "Xenial" LTS and Debian 9 "stretch" On master, drop Ubuntu 16.04 "Xenial" LTS and Debian 9 "stretch" Aug 31, 2021
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.

I'd prefer we change the scripts back to using /bin/bash instead of using /usr/bin/env bash; but I'm not going to block. See my comment on the one case for more info/discussion.

@@ -1,4 +1,4 @@
#!/bin/bash
#!/usr/bin/env bash
Copy link
Member

Choose a reason for hiding this comment

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

/bin/bash is actually guaranteed to be there per Linux Standard Base.
/usr/bin/env is not.

There's various arguments either way for most things, and most scripts engines it can make sense. However, /bin/bash and /bin/sh are two that have to be around for a basic system to function and can be directly relied upon so they don't make sense to export out to /usr/bin/env.

$0.02

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. I can look this up later, I suppose. Anyway, thanks for approving the PR

@stephengtuggy stephengtuggy merged commit b8cd738 into vegastrike:master Sep 1, 2021
@stephengtuggy stephengtuggy moved this from In progress to Done in 0.9.x Release Sep 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
3 participants