Skip to content

Switch libtscore to static and deal with the fallout#9974

Merged
bneradt merged 8 commits intoapache:masterfrom
cmcfarlen:cmake-static-libs
Jul 13, 2023
Merged

Switch libtscore to static and deal with the fallout#9974
bneradt merged 8 commits intoapache:masterfrom
cmcfarlen:cmake-static-libs

Conversation

@cmcfarlen
Copy link
Contributor

@cmcfarlen cmcfarlen commented Jul 7, 2023 •
edited by zwoop
Loading

No description provided.

@zwoop zwoop changed the title switch libraries to static and deal with the fallout Switch libtscore to static and deal with the fallout Jul 8, 2023
@zwoop
Copy link
Contributor

zwoop commented Jul 8, 2023

I'm working on the corresponding changes to automake. I drew the short straw.

@cmcfarlen
Copy link
Contributor Author

cmcfarlen commented Jul 10, 2023

[approve ci ubuntu]

@cmcfarlen cmcfarlen self-assigned this Jul 10, 2023
@cmcfarlen cmcfarlen added the CMake work related to CMakes scripts or issues label Jul 10, 2023
@cmcfarlen cmcfarlen added this to the 10.0.0 milestone Jul 10, 2023
@cmcfarlen
Copy link
Contributor Author

cmcfarlen commented Jul 10, 2023

[approve ci centos]

@cmcfarlen cmcfarlen marked this pull request as ready for review July 10, 2023 15:12
@ywkaras
Copy link
Contributor

ywkaras commented Jul 10, 2023

@SolidWallOfCode and @dragon512 may want to chime in on this.

@JosiahWI JosiahWI self-requested a review July 10, 2023 18:29
@JosiahWI
Copy link
Contributor

JosiahWI commented Jul 10, 2023 •
edited
Loading

It looks like some targets are linking to pcap after these changes that don't use it. Could we mark those with a # transitive comment? If we don't keep track of which usage requirements are transitive, then figuring it out later will be a pain.

@zwoop
Copy link
Contributor

zwoop commented Jul 10, 2023

I've tested this with my Makefile.am branch, and it works fine. I will make a PR for my branch as soon as this lands, since there are dependencies.

@zwoop
Copy link
Contributor

zwoop commented Jul 10, 2023

It looks like some targets are linking to pcap after these changes that don't use it. Could we mark those with a # transitive comment? If we don't keep track of which usage requirements are transitive, then figuring it out later will be a pain.

Yeh, we can go back and fix those once the larger refactoring is done. The Makefile patch I have is large, 2000 lines of changes.

@zwoop zwoop mentioned this pull request Jul 10, 2023
Merged
@JosiahWI
Copy link
Contributor

JosiahWI commented Jul 10, 2023

It looks like some targets are linking to pcap after these changes that don't use it. Could we mark those with a # transitive comment? If we don't keep track of which usage requirements are transitive, then figuring it out later will be a pain.

Yeh, we can go back and fix those once the larger refactoring is done. The Makefile patch I have is large, 2000 lines of changes.

Sounds good. Note that that my comment only applied to the CMake build, though. Automake doesn't manage usage requirements the same way.

@bryancall
Copy link
Contributor

bryancall commented Jul 10, 2023

[approve ci rocky]

@bryancall bryancall requested a review from bneradt July 10, 2023 22:29
@cmcfarlen cmcfarlen force-pushed the cmake-static-libs branch from dad607a to 8a31134 Compare July 11, 2023 16:17
@zwoop zwoop requested a review from bryancall July 11, 2023 19:06
@cmcfarlen cmcfarlen force-pushed the cmake-static-libs branch from 8a31134 to 05f422c Compare July 11, 2023 19:07
@JosiahWI
Copy link
Contributor

JosiahWI commented Jul 12, 2023

It looks like some targets are linking to pcap after these changes that don't use it. Could we mark those with a # transitive comment? If we don't keep track of which usage requirements are transitive, then figuring it out later will be a pain.

@JosiahWI JosiahWI closed this Jul 12, 2023
@JosiahWI JosiahWI reopened this Jul 12, 2023
JosiahWI
JosiahWI reviewed Jul 12, 2023
View reviewed changes
JosiahWI
JosiahWI requested changes Jul 12, 2023
View reviewed changes
Copy link
Contributor

@JosiahWI JosiahWI left a comment

Choose a reason for hiding this comment

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

Please address the above concern before this is merged.

JosiahWI
JosiahWI reviewed Jul 13, 2023
View reviewed changes
Copy link
Contributor

@JosiahWI JosiahWI left a comment

Choose a reason for hiding this comment

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

The CMake part of it looks pretty good now for a start. I'm planning to go through and try to improve the rest of the usage requirements separately at some point.

@JosiahWI JosiahWI self-requested a review July 13, 2023 02:35
@cmcfarlen
Copy link
Contributor Author

cmcfarlen commented Jul 13, 2023

[approve ci autest]

1 similar comment
@cmcfarlen
Copy link
Contributor Author

cmcfarlen commented Jul 13, 2023

[approve ci autest]

@bneradt bneradt merged commit c1093a4 into apache:master Jul 13, 2023
@cmcfarlen cmcfarlen deleted the cmake-static-libs branch July 13, 2023 15:38
bneradt added a commit to bneradt/trafficserver that referenced this pull request Nov 26, 2024
@bneradt
Revert "Switch libtscore to static and deal with the fallout (apache#…
41b7385 
…9974)"

But just source code changes.

This reverts commit c1093a4.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bneradt bneradt bneradt approved these changes

@bryancall bryancall Awaiting requested review from bryancall

@JosiahWI JosiahWI Awaiting requested review from JosiahWI

Assignees

@cmcfarlen cmcfarlen

Labels

CMake work related to CMakes scripts or issues

Projects

None yet

Milestone

10.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@cmcfarlen @zwoop @ywkaras @JosiahWI @bryancall @bneradt