Skip to content

Removes the UglyStub file #9401

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
Merged
zwoop merged 3 commits into from
Feb 14, 2023
Merged

Removes the UglyStub file #9401

zwoop merged 3 commits into from
Feb 14, 2023

Conversation

@zwoop
Copy link
Contributor

@zwoop zwoop commented Feb 12, 2023

With all the cleanup, and refactoring etc., there are only a small number of things that needs the ugly stubs. Fixing those allowed us to eliminate the UglyLogStubs library completely.

@zwoop zwoop added Build work related to build configuration or environment Cleanup labels Feb 12, 2023
@zwoop zwoop added this to the 10.0.0 milestone Feb 12, 2023
@zwoop zwoop requested review from brbzull0 and cmcfarlen February 12, 2023 17:39
@zwoop zwoop self-assigned this Feb 12, 2023
@zwoop
Copy link
Contributor Author

zwoop commented Feb 12, 2023

Well, seems linux builds are different than OSX builds for UDP. Looking into the issues.

@zwoop zwoop marked this pull request as draft February 13, 2023 16:29
@zwoop
Copy link
Contributor Author

zwoop commented Feb 13, 2023

Changing this to a draft, while I iterate over fixing all the oddities.

@zwoop zwoop force-pushed the UglyStubs branch 9 times, most recently from 4ca7a4d to ff7a9c1 Compare February 13, 2023 22:44
@zwoop zwoop marked this pull request as ready for review February 13, 2023 22:54
@bryancall bryancall requested a review from bneradt February 13, 2023 23:11
zwoop added 3 commits February 13, 2023 16:13
@zwoop
Removes the UglyStub file
d85b833 
With all the cleanup, and refactoring etc., there are only a small
number of things that needs the ugly stubs. Fixing those allowed
us to eliminate the UglyLogStubs library completely.
@zwoop
Fixes the build issues on Linux platforms
1299b6b
@zwoop
Fixes a few of the unit tests
3a12884
@zwoop
Copy link
Contributor Author

zwoop commented Feb 14, 2023

I think with this landed, we may be able to disconnect the dependencies on the udpPacketAllocator, but doing so requires us to move some "inline" functions into proper classes (as static). Can't quite get it to work though, but landing this will at least put us in a better position to clean up this mess with the UDP dependencies.

I also think that resolving this may help fixing "make check" problems.

@cmcfarlen
Copy link
Contributor

cmcfarlen commented Feb 14, 2023

We could move new_UDPPacket and UDPPacketInternal::free into UnixUDPNet.cc and then remove the extern from the P_UDPPacket.h file. The other new_ functions could just call new_UDPPacket instead of the allocator directly.

@cmcfarlen
Copy link
Contributor

cmcfarlen commented Feb 14, 2023

I really support this work as it's currently very complicated to add unit tests to much of the code. Any little bit helps.

@zwoop
Copy link
Contributor Author

zwoop commented Feb 14, 2023

We could move new_UDPPacket and UDPPacketInternal::free into UnixUDPNet.cc and then remove the extern from the P_UDPPacket.h file. The other new_ functions could just call new_UDPPacket instead of the allocator directly.

Yeh, I was trying that last night, and yes, I think this is right path, but ran into other problems. I think we should land this first, and then continue from there. I also think fixing all this will likely fix the "make check" issues on macOS.

@zwoop zwoop merged commit 20709e3 into apache:master Feb 14, 2023
@zwoop zwoop deleted the UglyStubs branch February 14, 2023 20:52
@cmcfarlen cmcfarlen mentioned this pull request Feb 15, 2023
Merged
cmcfarlen pushed a commit to cmcfarlen/trafficserver that referenced this pull request Jun 3, 2024
@zwoop
Merge remote-tracking branch 'asf/master'
03b605e 
* asf/master: (49 commits)
  Cleanup: removing some traffic_manager leftovers. (apache#9425)
  records.yaml - Add support to load multiple YAML docs from the same file and let traffic_ctl to modify a records.yaml file (apache#9404)
  QUIC-quiche: Use configured disable_active_migration param at quiche transport configuration. (apache#9447)
  records.yaml: Make sure we fail if we found a legacy records.config file. (apache#9435)
  Use TSDbg in webp_transform plugin (apache#9439)
  quic: make sure we create a stream if none available. (apache#9436)
  Fixes comparison with the wrong type (apache#9441)
  On arm64 macOS, do not use pagezero linker flag when using luajit (apache#9430)
  tscore: Remove unneeded and mispelled libswoc reference. (apache#9429)
  Remove UDP_stubs.h (apache#9413)
  Make jsonrcp restricted_api false by default. (apache#9415)
  Histogram: rename members because Zwoop. (apache#9417)
  FileManager string update (apache#9416)
  P_SSLUtils.h include updates (apache#9414)
  libswoc: update build support to fix issues with 10-Dev merge. (apache#9397)
  QUIC: Remove hardcoded quiche set_initial_max_streams and use config values instead. (apache#9412)
  Fixes the compile to work with LLVM15 (apache#9410)
  update cmake for rpc and swoc (apache#9409)
  Removes the UglyStub file (apache#9401)
  TSan: Make Thread::cur_time thread local (apache#9184)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cmcfarlen cmcfarlen cmcfarlen approved these changes

@bneradt bneradt bneradt approved these changes

@brbzull0 brbzull0 Awaiting requested review from brbzull0

Assignees

@zwoop zwoop

Labels

Build work related to build configuration or environment Cleanup

Projects

None yet

Milestone

10.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@zwoop @cmcfarlen @bneradt