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.

Sign up for GitHub

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

Jump to bottom

update genieutils #5

Merged
merged 1 commit into from
Feb 11, 2021
Merged

update genieutils #5

merged 1 commit into from
Feb 11, 2021
+6 −12

Conversation

FranciscoDA
Copy link
Contributor

@FranciscoDA FranciscoDA commented Jan 27, 2021 •
edited
Loading

update genieutils to work with latest DE patch thanks to latest changes in upstream

patch genie using a patch file

add patch to fix find_library() syntax in genie

@FranciscoDA FranciscoDA mentioned this pull request Jan 27, 2021
Open
HSZemi
HSZemi reviewed Jan 28, 2021
View reviewed changes
README.md Outdated
@@ -28,7 +28,7 @@ Just install stuff until it stops complaining I guess, sorry.
```sh
git clone --recurse-submodules https://github.com/SiegeEngineers/auto-mods.git
cd auto-mods
./patchGenieutils.sh
patch -d genieutils <genieutils.patch
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
patch -d genieutils <genieutils.patch
patch -d genieutils < genieutils.patch

genieutils.patch Outdated
Comment on lines 14 to 15
-find_library(iconv REQUIRED)
+find_library(iconv iconv REQUIRED)
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I am a cmake noob, what does this change do?

Copy link
Contributor

@simonsan simonsan Jan 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

https://cmake.org/cmake/help/latest/command/find_library.html

I think it just defines it shorthand as it should be so it states the <VAR> and a name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Related #1 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually also essential to find libiconv on MSYS/MingW64

Copy link
Contributor

Choose a reason for hiding this comment

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

If we stick to the format-patch/apply-patch approach and not the script, this should be:

Suggested change
-find_library(iconv REQUIRED)
+find_library(iconv iconv REQUIRED)
-find_library(iconv REQUIRED)
+find_package(Iconv REQUIRED)

Copy link
Contributor

Choose a reason for hiding this comment

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

find_package should be the preferred method as it's more high-level. As long as that package is deployed with the CMake version. We have to update cmake_minimum_required(VERSION 3.10) to cmake_minimum_required(VERSION 3.11) though for this change. But it will close #1 as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened Upstream PR https://github.com/Tapsa/genieutils/pull/3 as that's not an issue with us but in general.

Copy link
Contributor

@simonsan simonsan Jan 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

https://github.com/Tapsa/genieutils/blob/master/CMakeLists.txt#L19 and https://github.com/Tapsa/genieutils/blob/master/CMakeLists.txt#L35

Which seems strange to me. Could be that we don't even need to patch this anymore if Iconv is required as an upstream package.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably superseded by https://github.com/Tapsa/genieutils/pull/5

Copy link
Contributor

Choose a reason for hiding this comment

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

Tapsa merged @FranciscoDA @HSZemi

simonsan
simonsan reviewed Jan 29, 2021
View reviewed changes
genieutils.patch Outdated
Comment on lines 9 to 10
-#set(Boost_USE_STATIC_LIBS ON)
+set(Boost_USE_STATIC_LIBS ON)
Copy link
Contributor

@simonsan simonsan Jan 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I remember we had problems because of this @HSZemi when we wanted to statically link to boost, no?

And is Boost and everything now statically link as a standard? I thought it was only for distribution reasons to do that once? Now it's the normal way of linking from looking at the readme?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This patch is already in the patchGenieutils.sh file found in master. I simply executed the bash script and generated this patch file with git diff so the end result should be the same

Copy link
Contributor

Choose a reason for hiding this comment

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

True, it's less a review comment then a question to Hszemi, if it's still said that the project should be statically linked in general and not just for special purposes.

Copy link
Member

Choose a reason for hiding this comment

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

I need a statically linked binary that I can copy ~wherever~ in order to automatically update the random costs mods daily.
I currently see no benefit in additionally maintaining a dynamically linked version 🤷

@FranciscoDA FranciscoDA force-pushed the updateGenie branch 2 times, most recently from 11be212 to 3acac7d Compare January 29, 2021 01:57
This was referenced Jan 29, 2021
Draft
Draft
Closed
@simonsan
Copy link
Contributor

simonsan commented Jan 29, 2021 •
edited
Loading

I'm honestly not sure if that sed-script isn't better long-term because it's more flexible than a static patch given the fact these submodules are not controlled by us. @HSZemi @FranciscoDA

@FranciscoDA
Copy link
Contributor Author

FranciscoDA commented Jan 29, 2021 •
edited
Loading

I'm honestly not sure if that sed-script isn't better long-term because it's more flexible than a static patch given the fact these submodules are not controlled by us. @HSZemi @FranciscoDA

IMHO, patch files are more robust in this use case since it will not replace unwanted lines (I'm thinking about accidental multiple matches) and it will fail/warn if a hunk was not patched.

In short, I think a patching mechanism that fails in an unexpected situation is desirable over one that carries on without a hitch

@HSZemi
Copy link
Member

HSZemi commented Jan 29, 2021 •
edited
Loading

I agree that using patch is desirable over using the sed script 👍

@simonsan
Copy link
Contributor

Ok, makes sense to me as well.

@simonsan
Copy link
Contributor

simonsan commented Jan 29, 2021 •
edited
Loading

We should wait a bit, I fixed this upstream with a CMake flag like STATIC_COMPILE which we could use when calling cmake -DSTATIC_COMPILE=TRUE ..

Tracking https://github.com/Tapsa/genieutils/pull/5

Meaning no need for patching anymore.

simonsan
simonsan reviewed Jan 29, 2021
View reviewed changes
README.md Outdated
@@ -52,7 +52,7 @@ zlib1g-dev
Inside the `repository root` use the following commands:

```sh
./patchGenieutils.sh
patch -d genieutils < genieutils.patch
mkdir build
cd build
cmake ..
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cmake ..
cmake -DSTATIC_COMPILE=TRUE ..

Copy link
Contributor

Choose a reason for hiding this comment

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

This still needs to be added as a flag for CMake

Copy link
Contributor

Choose a reason for hiding this comment

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

And genieutils submodule needs to be updated to commit 6297d093db063bb0c6a3d69a3cc82aebfa88a1c6

simonsan
simonsan reviewed Jan 29, 2021
View reviewed changes
README.md Outdated
./patchGenieutils.sh
patch -d genieutils < genieutils.patch
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
./patchGenieutils.sh
patch -d genieutils < genieutils.patch

@FranciscoDA
Copy link
Contributor Author

@simonsan thanks, I've updated the genieutils submodule and the README.md file accordingly

simonsan
simonsan approved these changes Feb 11, 2021
View reviewed changes
Copy link
Contributor

@simonsan simonsan left a comment

Choose a reason for hiding this comment

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

Cool! ;)

@HSZemi HSZemi merged commit ff2f0a2 into SiegeEngineers:master Feb 11, 2021
@HSZemi
Copy link
Member

HSZemi commented Feb 11, 2021

Excellent stuff, thanks a lot 💛

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HSZemi HSZemi

@simonsan simonsan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@FranciscoDA @simonsan @HSZemi