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

Move externals to third_party and package licenses #375

Closed
wants to merge 2 commits into from
Closed

Move externals to third_party and package licenses #375

wants to merge 2 commits into from

Conversation

JeodC
Copy link
Collaborator

@JeodC JeodC commented May 20, 2024

Pull Request Type

  • GitHub Workflow changes
  • Documentation or Wiki changes
  • Build and Dependency changes
  • Runtime changes
    • Render changes
    • Audio changes
    • Input changes
    • Network changes
    • Other changes

Description

This pull piggybacks on #301 and consolidates.

  • External dependencies moved to third_party directory
  • ${subdir}_LICENSE.txt files created for each external dependency inside third_party
  • License files are added to Descent3/licenses/ at build time
  • CMakeLists.txt includes third_party headers under include_directories

Checklist

  • I have tested my changes locally and verified that they work as intended.
  • I have documented any new or modified functionality.
  • I have reviewed the changes to ensure they do not introduce any unnecessary complexity or duplicate code.
  • I understand that by submitting this pull request, I am agreeing to license my contributions under the project's license.

@JeodC JeodC added cleanup Code cleanup build Modification to the build system labels May 20, 2024
@JeodC JeodC requested a review from winterheart May 20, 2024 19:19
@winterheart
Copy link
Collaborator

Isn't MD5 implementation is our now? We don't have to make it as third_party. As it have same license and same copyright owners, it will be integral part of project.

winterheart
winterheart reviewed May 22, 2024
View reviewed changes
third_party/CMakeLists.txt
Comment on lines +5 to +29
add_custom_target(LicenseFiles ALL
COMMENT "Copying license files for third-party dependencies"
)

# Function to add third-party license
function(add_third_party_license license_name)
add_custom_command(TARGET LicenseFiles POST_BUILD
COMMAND ${CMAKE_COMMAND} -E make_directory "$<TARGET_FILE_DIR:Descent3>/licenses"
COMMAND ${CMAKE_COMMAND} -E copy
"${CMAKE_SOURCE_DIR}/third_party/${license_name}"
"$<TARGET_FILE_DIR:Descent3>/licenses/${license_name}"
COMMENT "Copying ${license_name}"
)
endfunction()

add_custom_command(TARGET LicenseFiles POST_BUILD
COMMAND ${CMAKE_COMMAND} -E copy
"${CMAKE_SOURCE_DIR}/LICENSE"
"$<TARGET_FILE_DIR:Descent3>/GPL-3.txt"
COMMENT "Copying Descent3 license"
)

add_third_party_license(libacm_LICENSE.txt)
add_third_party_license(md5_LICENSE.txt)
add_third_party_license(stb_LICENSE.txt)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be done as installation process, not a building process. See #318.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I consider that pull to be a "future feature" thing, as commented. It's common for game sources to be distributed and built as a portable option--I actually have never seen cmake install used in practice. Do you mind if this bit remains for now and it can be adjusted in #318 if requested there?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with winterheart. cmake install is used for packaging, it is absolutely used practice, and that's when we need licenses copied. This change needs reverted

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But #301 was fine?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I never approved #301 and you merged it forcefully.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When you dropped off the face of the earth without a word. Before that, your comments on it were enthusiastic. I will not entertain this discussion on an open forum.

Copy link
Collaborator

Choose a reason for hiding this comment

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

apologies for having a personal life and other commitments.

@JeodC
Copy link
Collaborator Author

JeodC commented May 22, 2024

Isn't MD5 implementation is our now? We don't have to make it as third_party. As it have same license and same copyright owners, it will be integral part of project.

Except from a Discord conversation with its author:

Jeod — 05/20/2024 9:03 AM
@Parabolicus What did you license md5 under again?
Parabolicus — 05/20/2024 9:05 AM
it is my original code so whatever license works
Jeod — 05/20/2024 9:06 AM
I was going to put it in the third_party dir, unless you'd say it isn't really an external dependency.
Up to you.
@Parabolicus Do you mind briefly checking out 375 and let me know if you want any changes to the md5 license?
I kept it third party so others could feel more inclined to copy and use it for their own projects (not tying it to Descent 3 explicitly).
Parabolicus — 05/20/2024 9:58 AM
no objections

Lgt2x
Lgt2x requested changes May 22, 2024
View reviewed changes
Copy link
Collaborator

@Lgt2x Lgt2x left a comment

Choose a reason for hiding this comment

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

MD5 is not a third-party, it has no upstream, we maintain it, and it was written explicitly for this project

third_party/CMakeLists.txt
Comment on lines +5 to +29
add_custom_target(LicenseFiles ALL
COMMENT "Copying license files for third-party dependencies"
)

# Function to add third-party license
function(add_third_party_license license_name)
add_custom_command(TARGET LicenseFiles POST_BUILD
COMMAND ${CMAKE_COMMAND} -E make_directory "$<TARGET_FILE_DIR:Descent3>/licenses"
COMMAND ${CMAKE_COMMAND} -E copy
"${CMAKE_SOURCE_DIR}/third_party/${license_name}"
"$<TARGET_FILE_DIR:Descent3>/licenses/${license_name}"
COMMENT "Copying ${license_name}"
)
endfunction()

add_custom_command(TARGET LicenseFiles POST_BUILD
COMMAND ${CMAKE_COMMAND} -E copy
"${CMAKE_SOURCE_DIR}/LICENSE"
"$<TARGET_FILE_DIR:Descent3>/GPL-3.txt"
COMMENT "Copying Descent3 license"
)

add_third_party_license(libacm_LICENSE.txt)
add_third_party_license(md5_LICENSE.txt)
add_third_party_license(stb_LICENSE.txt)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with winterheart. cmake install is used for packaging, it is absolutely used practice, and that's when we need licenses copied. This change needs reverted

@JeodC
Copy link
Collaborator Author

JeodC commented May 22, 2024

MD5 is not a third-party, it has no upstream, we maintain it, and it was written explicitly for this project

No we do not maintain it. We certainly can, but ultimately a non-organization member wrote it. You're welcome to get further clarification from @ziplantil.

@Lgt2x
Copy link
Collaborator

Lgt2x commented May 22, 2024

MD5 is not a third-party, it has no upstream, we maintain it, and it was written explicitly for this project

No we do not maintain it. We certainly can, but ultimately a non-organization member wrote it. You're welcome to get further clarification from @ziplantil.

A 3rd party has an UPSTREAM repository where we can pull changes from. This is not the case here, because it was written for this project, hence not a 3rd party.

@JeodC JeodC closed this May 22, 2024
@JeodC JeodC deleted the pack-licenses branch May 22, 2024 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@winterheart winterheart winterheart left review comments

@Lgt2x Lgt2x Lgt2x requested changes

Assignees
No one assigned
Labels
build Modification to the build system cleanup Code cleanup
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@JeodC @winterheart @Lgt2x