search

LandSandBoat / server

:sailboat: LandSandBoat - a server emulator for Final Fantasy XI
https://landsandboat.github.io/server/
GNU General Public License v3.0
301 stars 612 forks source link

Minor edit #6402

Closed DCS78 closed 3 weeks ago

DCS78 commented 3 weeks ago

I affirm:

What does this pull request do?

CMake minimum - 3.20 => 3.29; target_compile_features - 17 => 20; updated external file versions to latest versions that does not cause an error; blowfish.py - 2002 => 2015 version

Steps to test these changes

Delete the project cache before rebuild. No changes ingame.

Xaver-DaRed commented 3 weeks ago

What is the issue you are trying to solve?

DCS78 commented 2 weeks ago

Thank you for the feedback.

This was the first commit I have ever made so I wanted to start small and test the waters.

There are, generally, two camps: -"it works, why change it" -"modernize"

I am attempting to unify the code base on .net 8 and remove some obsolete pieces. I realize my scope is narrow (only testing on Win11) so, it may not work globally I can put it out there for veteran programmers to decide.

As for why I bump versions. Usually, newer versions include optimizations as well as new features that maybe employed.

Partially, I am doing this to try to solve my issue of the map program randomly crashing. I have not pinned down a pattern, yet.

It is clear that I need to learn and adjust to community standards such as space at the end of files. I will look at my settings but I'm sure that was just me. As for the version, I will need to look again at the references that had it versus the ones that didn't to see what is different.

Again, thank you for the feedback.

Sent via the Samsung Galaxy S24 Ultra, an AT&T 5G smartphone

-------- Original message -------- From: Zach Toogood @.> Date: 11/3/24 12:13 PM (GMT-08:00) To: LandSandBoat/server @.> Cc: Dave @.>, Author @.> Subject: Re: [LandSandBoat/server] Minor edit (PR #6402)

@zach2good commented on this pull request.

👋 Hello and welcome!

tools/headlessxi/blowfish.py - moved to 2002 to 2015 version

If it works, why is this being changed?

In CMakeLists.txthttps://github.com/LandSandBoat/server/pull/6402#discussion_r1827056161:

@@ -135,4 +139,4 @@ endif()

include_directories(${CMAKE_BINARY_DIR}/generated) # Globally include the build/generated directory

-add_subdirectory(src) +add_subdirectory(src)

Your editor is probably removing newlines on save. We keep newlines at the end of files.

In CMakeLists.txthttps://github.com/LandSandBoat/server/pull/6402#discussion_r1827056173:

@@ -87,7 +91,7 @@ endif()

Link this 'library' to set the c++ standard / compile-time options requested

add_library(project_options INTERFACE) -target_compile_features(project_options INTERFACE cxx_std_17) +target_compile_features(project_options INTERFACE cxx_std_20)

👍

In CMakeLists.txthttps://github.com/LandSandBoat/server/pull/6402#discussion_r1827056444:

@@ -1,10 +1,14 @@ -cmake_minimum_required(VERSION 3.20) +cmake_minimum_required(VERSION 3.29)

What do we gain by bumping this? We should try to keep tooling versions as low as reasonably possible unless we have a reason to update.

In CMakeLists.txthttps://github.com/LandSandBoat/server/pull/6402#discussion_r1827056497:

project(server C CXX)

https://cmake.org/cmake/help/latest/policy/CMP0069.html

cmake_policy(SET CMP0069 NEW) set(CMAKE_POLICY_DEFAULT_CMP0069 NEW)

+# https://cmake.org/cmake/help/latest/policy/CMP0161.html

CMP0161 Added in version 3.29.

The CPACK_PRODUCTBUILD_DOMAINS variable defaults to true.

We don't use CPack, why do we care about this?

In ext/CMakeLists.txthttps://github.com/LandSandBoat/server/pull/6402#discussion_r1827056783:

  • GIT_TAG v1.14.1 #27cb4c76708608465c413f6d0e6b8d99a4d84302
  • VERSION 1.14.1

From the docs:

The origin may be specified by a GIT_REPOSITORY, but other sources, such as direct URLs, are also supported. If GIT_TAG hasn't been explicitly specified it defaults to v(VERSION), a common convention for git projects. On the other hand, if VERSION hasn't been explicitly specified, CPM can automatically identify the version from the git tag in some common cases. GIT_TAG can also be set to a specific commit or a branch name such as master, however this isn't recommended, as such packages will only be updated when the cache is cleared.

Is it redundant to specify both GIT_TAG and VERSION? I'd rather have just the tag and the comment with the commit hash, if we're not using just a specific hash

— Reply to this email directly, view it on GitHubhttps://github.com/LandSandBoat/server/pull/6402#pullrequestreview-2411934143, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AB72TFGKHTNUIVWLNGYVZ5DZ6Z7X5AVCNFSM6AAAAABRCF3SO2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMJRHEZTIMJUGM. You are receiving this because you authored the thread.Message ID: @.***>