Skip to content

Conversation

pjreiniger
Copy link
Contributor

This is the script I used to perform the big refactoring. It is a little bit unwieldy because it was built with duct tape and bubble gum and then I made an attempt to reorganize and clean it up. It is mildly commented, here is a general overview.

Whats included

This effort attempts to cover as much of the reorg spreadsheet as possible without major build system changes and without major code changes

Things covered in the script:

  • All subprojects but glass have been moved into their new folders
  • Java package renames (and paths in repo)
  • C++ file moves, as well as a vast change to using .hpp instead of .h as noted here
    • NOTE, A bulk assumption was made that all files, unless the files explicitly had _c in their name was to change the file. This might be incorrect.
  • C++ namespaces
  • Change maven coordinates
  • While not explicitly denoted in the spreadsheet, the C++ files were moved to attempt to match the java file moves.

Things NOT covered in the script, which I think are outside the scope of this initial effort

  • Separate glassApp from glasslib. This is a build system change so it will be done in a follow up PR.
  • Combine wpilibc and wpilibj into a common project. Again, build system change, follow up PR
  • Move C++ units into the wpiunits project. Again, build system
  • Some of the suggested file moves denoted in the spreadsheet are across project boundaries and need dependency chains broken before they can move
  • Per an offline conversation, the serde project might not bring a whole lot of benefit, so it has been left in wpiutil with namespaces / include paths unchanged.

Process

  1. Preprocess C++ and Java files to determine file moves, and calculate their
    respective changes to other files, i.e. how to change C++ include paths, or
    java package / import renames
    1b. Additional renames are applied as per this document https://docs.google.com/spreadsheets/d/1NXgby1njo7oZ9_Ezk8q6ez5VzPTXeLAVRSnB3M1TqR4/edit?gid=1323156307#gid=1323156307
    Note, some of the proposed renames require substantial build file modifications or need dependencies broken, and therefore are not covered with this script.
  2. Apply the pre-processed renames and include / import fixups
    3-N. Apply some manual fixes, as well as run scripts like the linters, upstream utils, pregenerated files, etc
    N-M. Apply namespace changes. This is a combination of automated find/replace as well as hand fixes.

An intermediate file, refactor_layout_pp.json is created to track the file moves and namespace / import changes. This file can be repurposed in a simpler script to help with the upgrade process for vendors / teams

Other notes

The namespace changes were non-trivial. Since some projects (wpiutil, wpinet shared wpi::, wpimath, wpilibc shared frc::) you cannot trivially do a bulk replacement and differentiate between what belongs to what new nested namespace. Furthermore, there is no consistency in C++ if files have using namespace XXX; vs namespace XXX {}. Also there was no consistency between when fully qualified names were used vs non fully qualified.

To address these things a couple of function / types are defined a prior to get replaced before bulk actions happen. In addition, everything, in both .cpp files and .h files will use fully qualified namespaces, regardless of the fact that they all now share a common wpi:: prefix.

Some of the regex's could probably be combined / simplified / more robust, but it works for a temporary script.

The script is not intended to be located inside of allwpilib, but I thought this would be the best way to review it.

@pjreiniger pjreiniger requested a review from a team as a code owner September 27, 2025 15:40
@github-actions github-actions bot added the 2027 2027 target label Sep 27, 2025
("cameraserver", "edu.wpi.first.cameraserver", "org.wpilib.vision.stream"),
("xrpVendordep", "edu.wpi.first.wpilibj.xrp", "org.wpilib.xrp"),
("romiVendordep", "edu.wpi.first.wpilibj.romi", "org.wpilib.romi"),
("wpilibjExamples", "edu.wpi.first.wpilibj.commands", "org.wpilib.commands"),
Copy link
Member

Choose a reason for hiding this comment

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

Maybe org.wpilib.snippets.commands2? It should have 2 in the package name, but also shouldn't use org.wpilib.commands2 since that would split the package which Java does not like.

The files in this package are also fairly useless; personally, I'd suggest just deleting it, but that'd out of scope for this PR

@PeterJohnson
Copy link
Member

PeterJohnson commented Sep 27, 2025

It looks like the .hpp renames didn't occur in the source directories (e.g. ntcore and cscore have a bunch of internal .h files in their source directories).

I think we'd also like to change all the <wpi/...> style includes into just using "wpi/..." everywhere (this should mostly just be a tweak to the styleguide externalLibs setting).

Looks like there's still some things to move in the wpilibj area that I missed in the spreadsheet? I know some of these may eventually move again or get removed.

  • DSControlWord - driverstation
  • SystemServer, RuntimeType, SensorUtil - system
  • SharpIR - hardware/range
    etc

@Gold856
Copy link
Contributor

Gold856 commented Sep 27, 2025

ntcoreffi's DataLogManager still has some frc namespaces. There's also a comment in wpi/main/Main.hpp for HAL_SetMain that refers to frc::StartRobot(). Multiple HAL JNI files have lines like auto stack = wpi::util::java::GetJavaStackTrace(env, "edu.wpi.first");. Similarly, DriverStation.reportErrorImpl needs to be updated to look for org.wpilib instead of edu.wpi.first. There's also an extra wpi directory in wpi/fields/wpi/fields.hpp. A bunch of the wpilibc source files didn't get moved. The struct and proto files for Matrix and Vector weren't moved with the Matrix and Vector files. GeneratedFiles.md should be updated to refer to the new Java packages. Also probably want to update the vendordep files?

@Gold856
Copy link
Contributor

Gold856 commented Sep 27, 2025

There's a few std:: shims that I think we want to keep at the top-level wpi:: namespace. print is one of them, but so are all of the polyfills for stuff like <expected> and <debugging>. There's probably a few others I missed, especially in LLVM.

@PeterJohnson
Copy link
Member

PeterJohnson commented Sep 27, 2025

The LLVM stuff is mostly not polyfill, so should likely stay in the wpi::util namespace? I guess the broader question is how "clean" do we want the wpi level namespace to be--should it be just true WPILib stuff or should it have container classes etc?

@pjreiniger
Copy link
Contributor Author

pjreiniger commented Sep 27, 2025

Looks like there's still some things to move in the wpilibj area that I missed in the spreadsheet? I know some of these may eventually move again or get removed.

  • DSControlWord - driverstation
  • SystemServer, RuntimeType, SensorUtil - system
  • SharpIR - hardware/range
    etc

After those, the following are left in the root package

  • Alert - system? util?
  • AnalogGyro - hardware/gyro? It would be the only thing in there. rotation feels wrong since its a different type of rotation
  • Notifier - system? That is one of the ones that was ideally moved to wpiutil but needs dependencies broken (import org.wpilib.hardware.hal.NotifierJNI, and then the native impl needs hal stuff as well);
  • OnboardIMU - hardware/imu? Also a single file
  • Preferences - system? util?

@pjreiniger
Copy link
Contributor Author

pjreiniger commented Sep 27, 2025

wpilibc has the same things, plus

  • WPIErrors.mac
  • WPIWarnings.mac

@PeterJohnson
Copy link
Member

AnalogGyro is going to go away (#8205) so don't worry about that one.
OnboardIMU should go to hardware/imu.
I think both Alert and Preferences should go to util
Yeah put Notifier in system for now until we can move it.

include_replacements["Color.h"] = "wpi/util/Color" + NEW_CC_FILE_SUFFIX
include_replacements["util/Color8Bit.h"] = "wpi/util/Color8Bit" + NEW_CC_FILE_SUFFIX
include_replacements["BooleanEvent.h"] = "wpi/event/BooleanEvent" + NEW_CC_FILE_SUFFIX
include_replacements["../../include/frc/controller/BangBangController.h"] = "wpi/math/controller/BangBangController" + NEW_CC_FILE_SUFFIX
Copy link
Contributor

Choose a reason for hiding this comment

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

... Weird. I'm just gonna clean it up separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

@PeterJohnson
Copy link
Member

PeterJohnson commented Oct 7, 2025

There's a fairly significant number of C headers with .hpp extensions. A good heuristic is that any .h file with a ifdef __cplusplus in it should remain .h. This includes ntcore.h (which is a bridge header to either the C++ or C versions), all of the HAL headers, one header in cscore (cscore_raw.h), a few headers in wpiutil, etc. Some of these headers do have a mix of both (e.g. wpiutil's string.h) so we could potentially split the C++ parts out to a separate .hpp file, but that can be done afterwards.

@PeterJohnson
Copy link
Member

Should the hpp renames be applied to thirdparty imports into wpi/ as well? We clearly shouldn't do the ones that can be either thirdparty or pull from vcpkg/system (e.g. libfmt), but llvm, json, sigslot, etc could arguably be renamed. Currently the fact that many of those are still .h leads to a pretty inconsistent feeling for the things in the wpi/ source tree. Understand that patching thirdparty stuff gets complicated.

@pjreiniger
Copy link
Contributor Author

My gut says leave them how they are upstream, but that isn't actually currently what is happening. For example json files get renamed from .hpp -> .h (I was going to do that in a follow up), so I wouldn't be crazy to do it in the other direction.

The original issue created by @calcmogul only really mentions wpimath and wpilib[c], so this could pivot towards only doing those libraries. Either way I think he should drop his opinion on it

@calcmogul
Copy link
Member

In my opinion, if an upstream library was mostly imported as-is, it should keep the upstream extensions. If it was modified to the point of essentially being a different library anyway, we can make the extensions whatever we want.

@pjreiniger
Copy link
Contributor Author

From quickly searching the upstream scripts, this is the state of things:

apriltag - originally .h
argparse - renamed from .hpp -> .h
benchmark - originally .h
catch2 - apparently landed as .hpp, breaking tradition
eigen - .h (and no suffix)
expected - renamed from .hpp to no suffix
glfw - originally .h
imgui - originally .h
implot - originally .h
json - originally hpp, renamed to .h
dogleg - originally .h
libuv - originally .h. Note: the .c files get renamed to cpp
llvm - originally .h
mpack originally .h
mrcal - original .h and .hh, all
nanopb - originally .h
sleipnir - .hpp, left alone
stb - .h
upb - .h

LLVM is the most patched up.

argparse, llvm, expected, json, mpack, sigslot have their include paths modified to make them feel like they are part of wpilib.

I assume LLVM, eigen and json are the most user facing libraries.

After saying all of that, I would lobby for leaving the upstreams how they come (i.e. revert json), but since LLVM is so hacked up and user facing, rename those files

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

Labels

2027 2027 target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants