diff --git a/AGENTS.md b/AGENTS.md index 0e8160a46d..c35a7e8958 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -33,8 +33,10 @@ instructions for your specific platform. * **CMake**: Version 3.7 or newer. * **Python**: Version 3.7 or newer. * **Abseil-py**: Python package. -* **OpenSSL**: Required for Realtime Database and Cloud Firestore (especially - for desktop builds). +* **OpenSSL**: Required for desktop builds, unless you build with the + `-DFIREBASE_USE_BORINGSSL=YES` cmake flag. +* **libsecret-1-dev**: (Linux Desktop) Required for secure credential storage. + Install using `sudo apt-get install libsecret-1-dev`. * **Android SDK & NDK**: Required for building Android libraries. `sdkmanager` can be used for installation. CMake for Android (version 3.10.2 recommended) is also needed. @@ -42,6 +44,12 @@ instructions for your specific platform. Android builds on Windows. * **Cocoapods**: Required for building iOS or tvOS libraries. +To build for Desktop, you can install prerequisites by running the following +script in the root of the repository: `scripts/gha/install_prereqs_desktop.py` + +To build for Android, you can install prerequisites by running the following +script in the root of the repository: `build_scripts/android/install_prereqs.sh` + ## Building the SDK The SDK uses CMake for C++ compilation and Gradle for Android-specific parts. @@ -50,15 +58,22 @@ The SDK uses CMake for C++ compilation and Gradle for Android-specific parts. 1. Create a build directory (e.g., `mkdir desktop_build && cd desktop_build`). 2. Run CMake to configure: `cmake ..` - * For iOS: - `cmake -DCMAKE_TOOLCHAIN_FILE=../cmake/toolchains/ios.cmake ..` - Note: iOS setup typically requires both including Firebase pods (via - `Podfile`) and linking the `.framework` files from the C++ SDK - distribution. - * For tvOS: - `cmake -DCMAKE_TOOLCHAIN_FILE=../cmake/toolchains/apple.toolchain.cmake -DPLATFORM=TVOS ..` + * For Desktop: Run as is. You can use BORINGSSL instead of OpenSSL (for fewer + system dependencies with the `-DFIREBASE_USE_BORINGSSL=YES` parameter. + * For iOS, include the `-DCMAKE_TOOLCHAIN_FILE=../cmake/toolchains/ios.cmake` + parameter. This requires running on a Mac build machine. 3. Build specific targets: `cmake --build . --target firebase_analytics` (replace `firebase_analytics` with the desired library). + Or omit the entire `--target` parameter to build all targets. + + For development, building specific targets + (e.g., `cmake --build . --target firebase_app`) is generally faster and + recommended once CMake configuration is complete. The full build + (`cmake --build .`) can be very time-consuming (but can be sped up by adding + `-j4` to the command-line). + +You can also use the `scripts/gha/build_desktop.py` script to build the full +desktop SDK. Refer to `README.md` for details on CMake generators and providing custom third-party dependency locations. @@ -76,17 +91,30 @@ This command should be run from the root of the repository. Proguard files are generated in each library's build directory (e.g., `analytics/build/analytics.pro`). -### Desktop Platform Setup Details +You can build the entire SDK for Android by running `./gradlew build` or +`build_scripts/android/build.sh`. + +### Xcode (iOS) + +Unfortunately, the iOS version of the SDK cannot be built on Linux, it can only +be built in a MacOS environment. You will have to rely on GitHub Actions to +build for iOS, and have the user inform you of any build issues that come up. + +### Troubleshooting Desktop Builds -When setting up for desktop, if you are using an iOS -`GoogleService-Info.plist` file, convert it to the required -`google-services-desktop.json` using the script: -`python generate_xml_from_google_services_json.py --plist -i GoogleService-Info.plist` -(run this from the script's directory, ensuring the plist file is accessible). +* Linux: **Missing `libsecret-1-dev`**: + CMake configuration may fail if `libsecret-1-dev` is not installed. + The `scripts/gha/install_prereqs_desktop.py` script should handle this. + If it doesn't, or if the package is removed, you might need to install it + manually: `sudo apt-get update && sudo apt-get install -y libsecret-1-dev`. -The desktop SDK searches for configuration files in the current working -directory, first for `google-services-desktop.json`, then for -`google-services.json`. +* Linux: **LevelDB Patch Failure when building Firestore**: + If you are building the SDK with Firestore enabled + (`-DFIREBASE_INCLUDE_FIRESTORE=ON`, which is the default for desktop) and + encounter a patch error related to `leveldb-1.23_windows_paths.patch` (e.g., + `util/env_windows.cc: patch does not apply`), you can ignore this issue if + it does not prevent the rest of the build from running. The patch is only + important on Windows. Common system library dependencies for desktop: * **Windows**: Common dependencies include `advapi32.lib`, `ws2_32.lib`, @@ -97,6 +125,9 @@ Common system library dependencies for desktop: * **Linux**: Common dependencies include `pthread` (system library). When using GCC 5+, define `-D_GLIBCXX_USE_CXX11_ABI=0`. +On all desktop platforms, building with -DFIREBASE_USE_BORINGSSL=YES can help +bypass any OpenSSL dependency issues. + ## Including the SDK in Projects ### CMake Projects @@ -139,18 +170,10 @@ coverage within the integration tests. (e.g., Firestore, Auth) are typically located in the `integration_test/` directory within that product's module (e.g., `firestore/integration_test/`). -* **Test Scripts**: The root of the repository contains scripts for running - tests on various platforms, such as: - * `test_windows_x32.bat` - * `test_windows_x64.bat` - * `test_linux.sh` - * `test_mac_x64.sh` - * `test_mac_ios.sh` - * `test_mac_ios_simulator.sh` - - These scripts typically build the SDKs and then execute the relevant tests - (primarily integration tests) via CTest or other platform-specific test - runners. + + Because building integration tests requires internal google-services files, + Jules cannot do it in its environment; instead, we rely on GitHub Actions's + Integration Test workflow to build and run the integration tests. ## Writing Tests @@ -221,6 +244,19 @@ where `T` is the type of the expected result. callback function (lambda or function pointer) that will be invoked when the future completes. The callback receives the completed future as an argument. +* k?????Fn_* enums: A list of each SDK's asynchronous functions is usually + kept in an enum in that SDK. For example, all of Auth's asynchronous + functions are named kAuthFn_* and kUserFn_*. Only asynchronous operations + (which return a Future) need to be in those function enums; these are used + internally to hold a reference to the FutureHandle for the *LastResult() + methods. If you add a new asynchronous operation, it should be added to + that enum, and that ID should be used for all of the internal FutureApi + operations. Non-async functions never need to touch this. +* Asynchronous functions ONLY: Only asynchronous functions need to use + the Future pattern, e.g. anything with a callback. If you are simply + calling an underlying SDK function that finishes its work and returns + immediately, with no callback, there is no need to use a Future. See + `STYLE_GUIDE.md` for more details on asynchronous operations. ### Core Classes and Operations (Examples from Auth and Database) @@ -306,6 +342,9 @@ API documentation. ## Coding Style +* **Firebase C++ Style Guide**: For specific C++ API design and coding + conventions relevant to this SDK, refer to the + [STYLE_GUIDE.md](STYLE_GUIDE.md). * **Google C++ Style Guide**: Adhere to the [Google C++ Style Guide](https://google.github.io/styleguide/cppguide.html) as mentioned in `CONTRIBUTING.md`. @@ -624,3 +663,17 @@ practices detailed in `AGENTS.md`. to locate files/modules based on common directory structures are unsuccessful, explicitly ask for more specific paths rather than assuming a module doesn't exist or contains no relevant code. + +## GitHub Code Review Comments -- IMPORTANT, JULES READ THIS SECTION + +If the user asks you to look at GitHub code review comments, PR reviews, etc, +that means that you should run the following command: + +`python3 scripts/print_github_reviews.py --branch BRANCH_NAME_THAT_JULES_PUSHED_TO_GITHUB` + +(with the remote branch name that was actually pushed to GitHub substituted, and +if you aren't sure what it is or it returns an error, just ask the user, as they +may have renamed the branch before pushing) and then address the comments that +the output shows. If it succeeds, then subsequent times you run the script on +the same branch, include the `--since` parameter in accordance with the previous +script run's output to ensure you only fetch new comments. diff --git a/STYLE_GUIDE.md b/STYLE_GUIDE.md new file mode 100644 index 0000000000..a141ee0b66 --- /dev/null +++ b/STYLE_GUIDE.md @@ -0,0 +1,378 @@ +# C++ API Guidelines + +**WIP** - *please feel free to improve* + +Intended for Firebase APIs, but also applicable to any C++ or Game APIs. + +# Code Style + +Please comply with the +[Google C++ Style Guide](https://google.github.io/styleguide/cppguide.html) +as much as possible. Refresh your memory of this document before you start :) + +# C++ API Design + +### Don't force any particular usage pattern upon the client. + +C++ is a huge language, with a great variety of ways in which things can be +done, compared to other languages. As a consequence, C++ projects can be very +particular about what features of the language they use or don't use, how they +represent their data, and structure their code. + +An API that forces the use of a feature or structure the client doesn't use +will be very unwelcome. A good API uses only the simplest common denominator +data types and features, and will be useable by all. This can generally be +done with minimal impact on your API’s simplicity, or at least should form +the baseline API. + +Examples of typical Do's and Don'ts: + +* Don't force the use of a particular data structure to supply or receive + data. Typical examples: + * `std::vector`: If the client doesn't have the data already in a + `std::vector` (or only wants to use part of a vector), they are forced + to copy/allocate a new one, and C++ programmers don't like unnecessary + copies/allocations. + Instead, your primary interface should always take a + `(const T *, size_t)` instead. You can still supply an optional helper + method that takes a `std::vector &` and then calls the former if + you anticipate it to be called very frequently. + * `std::string`: Unlike Java, these things aren't pooled, they're mutable + and copied. A common mistake is to take a `const std::string &` + argument, which forces all callers that supply a `const char *` to go + thru a strlen+malloc+copy that is possibly of no use to the callee. + Prefer to take a `const char *` instead for things that are + names/identifiers, especially if they possibly are compile-time + constant. If you're unsetting a string property, prefer to pass + nullptr rather than an empty string. (There are + [COW implementations](https://en.wikipedia.org/wiki/Copy-on-write), + but you can't rely on that). + * `std::map`: This is a costly data structure involving many + allocations. If all you wanted is for the caller to supply a list of + key/value pairs, take a `const char **` (yes, 2 stars!). Or + `const SimpleStruct *` instead, which allows the user to create this + data statically. +* Per-product configuration should be accomplished using an options struct + passed to the library's `firebase::::Initialize` function. + Default options should be provided by the options struct's default + constructor. The `Initialize` function should be overloaded with a version + that does not take the options struct (which is how the Google style guide + prefers that we pass default parameters). + + For example, + +```c++ + struct LibraryOptions { + LibraryOptions() : do_the_thing(false) {} + + bool do_the_thing; + }; + + InitResult Initialize(const App& app) { + return Initialize(app, LibraryOptions()); + } + InitResult Initialize(const App& app, const LibraryOptions& options); +``` + +* Don't make your client responsible for data you allocate or take ownership + of client data. Typical C++ APIs are written such that both the client + and the library own their own memory, and they take full responsibility + for managing it, regardless of what the other does. Any data exchanged is + typically done through weak references and copies. + An exception may be a file loading API where buffers exchanged may be + really big. If you are going to pass ownership, make this super obvious + in all comments and documentation (C++ programmers typically won't + expect it), and document which function should be used to free the data + (free, delete, or a custom one). + Alternatively, a simple way to pass ownership of a large new buffer to + the client is to ask the client to supply a std::string *, which you then + resize(), and write directly into its owned memory. This somewhat + violates the rule about use of std::string above, though. + +* Don't use exceptions. This one is worth mentioning separately. Though + exceptions are great in theory, in C++ hardly any libraries use them, and + many code-bases disallow them entirely. They also require the use of RTTI + which some environments turn off. Oh, yes, also don't use RTTI. + +* Go easy on templates when possible. Yes, they make your code more general, + but they also pull a lot of implementation detail into your API, lengthen + compile times and bloat binaries. In C++ they are glorified macros, so + they result in hard to understand errors, and can make correct use of + your API harder to understand. + +* Utilize C++11 features where appropriate. This project has adopted C++11, + and features such as `std::unique_ptr`, `std::shared_ptr`, + `std::make_unique`, and `std::move` are encouraged to improve code safety + and readability. However, avoid features from C++14 or newer standards. + +* Go easy on objectifying everything, and prefer value types. In languages + like Java it is common to give each "concept" your API deals with its own + class, such that methods on it have a nice home. In C++ this isn't + always desirable, because objects need to be managed, stored and + allocated, and you run into ownership/lifetime questions mentioned above. + Instead: + + * For simple data, prefer their management to happen in the parent + class that owns them. Actions on them are methods in the parent. If + at all possible, prefer not to refer to them by pointer/reference + (which creates ownership and lifetime issues) but by index/id, or + string if not performance sensitive (for example, when referring to + file resources, since the cost of loading a file dwarfs the cost of + a string lookup). + + * If you must create objects, and objects are not heavyweight (only + scalars and non-owned pointers), make use of these objects by value + (return by value, receive by const reference). This makes ownership + and lifetime management trivial and efficient. + +* If at all possible, don't depend on external libraries. C++ compilation, + linking, dependency management, testing (especially cross platform) are + generally way harder than any other language. Every dependency is a + potential source of build complexity, conflicts, efficiency issues, and + in general more dependencies means less adoption. + + * Don't pull in large libraries (e.g. BOOST) just for your + convenience, especially if their use is exposed in headers. + + * Only use external libraries that have hard to replicate essential + functionality (e.g. compression, serialization, image loading, + networking etc.). Make sure to only access them in implementation + files. + + * If possible, make a dependency optional, e.g. if what your API does + benefits from compression, make the client responsible for doing so, + or add an interface for it. Add sample glue code or an optional API + for working with the external library that is by default off in the + build files, and can be switched on if desired. + +* Take cross-platform-ness seriously: design the API to work on ALL + platforms even if you don't intend to supply implementations for all. + Hide platform issues in the implementation. Don't ever include platform + specific headers in your own headers. Have graceful fallback for + platforms you don't support, such that some level of building / testing + can happen anywhere. + +* If your API is meant to be VERY widespread in use, VERY general, and very + simple (e.g. a compression API), consider making at least the API (if + not all of it) in C, as you'll reach an even wider audience. C has a + more consistent ABI and is easier to access from a variety of systems / + languages. This is especially useful if the library implementation is + intended to be provided in binary. + +* Be careful not to to use idioms from other languages that may be foreign + to C++. + + * An example of this is a "Builder" API (common in Java). Prefer to + use regular constructors, with additional set_ methods for less + common parameters if the number of them gets overwhelming. + +* Do not expose your own UI to the user as part of an API. Give the + developer the data to work with, and let them handle displaying it to + the user in the way they see fit. + + * Rare exceptions can be made to this rule on a case-by-case basis. + For example, authentication libraries may need to display a sign-in + UI for the user to enter their credentials. Your API may work with + data owned by Google or by the user (e.g. the user's contacts) that + we don't want to expose to the app; in those cases, it is + appropriate to expose a UI (but to limit the scope of the UI to the + minimum necessary). + + * In these types of exceptional cases, the UI should be in an isolated + component, separate from the rest of the API. We do allow UIs to be + exposed to the user UI-specific libraries, e.g. FirebaseUI, which + should be open-source so developers can apply any customizations + they need. + +# Game API Design + +### Performance matters + +Most of this is already encoded in C++ API design above, but it bears +repeating: C++ game programmers can be more fanatic about performance than +you expect. + +It is easy to add a layer of usability on top of fast code, it is very +hard to impossible to "add performance" to an API that has performance +issues baked into its design. + +### Don't rely on state persisting for longer than one frame. + +Games have an interesting program structure very unlike apps or web pages: +they do all processing (and rendering) of almost all functionality of the +game within a *frame* (usually 1/60th of a second), and then start anew +for the next frame. + +It is common for all or part of the state of a game to be wiped out from +one frame to the next (e.g when going into the menu, loading a new level, +starting a cut-scene..). + +The consequence of this is that the state kept between frames is the only +record of what is currently going on, and that managing this state is a +source of complexity, especially when part of it is reflected in external +code: + +* Prefer API design that is stateless, or if it is stateful, is so only + within a frame (i.e. between the start of the frame and the start of + the next one). This really simplifies the client's use of your API: + they can't forget to "reset" your API's state whenever they change + state themselves. + +* Prefer not to use cross-frame callbacks at all (non-escaping callbacks + are fine). Callbacks can be problematic in other contexts, but they're + even more problematic in games. Since they will execute at a future + time, there's no guarantee that the state that was there when the + callback started will still be there. There's no easy way to robustly + "clear" pending callbacks that don't make sense anymore when state + changes. Instead, make your API based on *polling*. + Yes, everyone learned in school that polling is bad because it uses + CPU, but that's what games are based on anyway: they check a LOT of + things every frame (and only at 60hz, which is very friendly compared + to busy-wait polling). If your API can be in various states of a state + machine (e.g. a networking based API), make sure the client can poll + the state you're in. This can then easily be translated to user + feedback. + If you have to use asynchronous callbacks, see the section on async + operations below. + +* Be robust to the client needing to change state. If work done in your + API involves multiple steps, and the client never gets to the end of + those steps before starting a new sequence, don't be "stuck", but deal + with this gracefully. If the game's state got reset, it will have no + record of what it was doing before. Try to not make the client + responsible for knowing what it was doing. + +* Interaction with threading: + + * If you are going to use threading at all, make sure the use of that + is internal to the library, and any issues of thread-safety don't + leak into the client. Allow what appears to be synchronous access + to a possibly asynchronous implementation. If the asynchronous + nature will be externally visible, see the section on async + operations below. + + * Games are typically hard to thread (since it’s hard to combine with + its per-frame nature), so the client typically should have full + control over it: it is often better to make a fully synchronous + single-threaded library and leave threading it to the client. Do + not try to make your API itself thread-safe, as your API is + unlikely the threading boundary (if your client is threaded, use + of your library is typically isolated to one thread, and they do + not want to pay for synchronization cost they don't use). + + * When you do spin up threads to reduce a workload, it is often a + good idea to do that once per frame, as avoid the above mentioned + state based problems, and while starting threads isn't cheap, you + may find it not a problem to do 60x per second. Alternatively you + can pool them, and make sure you have an explicit way to wait for + their idleness at the end of a frame. + +* Games typically use their own memory allocator (for efficiency, but + also to be able to control and budget usage on memory constrained + systems). For this reason, most game APIs tend to provide allocation + hooks that will be used for all internal allocation. This is even more + important if you wish to be able to transfer ownership of memory. + +* Generally prefer solutions that are low on total memory usage. Games + are always constrained on memory, and having your game be killed by + the OS because the library you use has decided it is efficient to + cache everything is problematic. + + * Prefer to recompute values when possible. + + * When you do cache, give the client control over total memory used + for this purpose. + + * Your memory usage should be predictable and ideally have no peaks. + +# Async Operations + +### Application Initiated Async Operations + +* Use the Future / State Pattern. +* Add a `*LastResult()` method for each async operation method to allow + the caller to poll and not save state. + +e.g. + +```c++ + // Start async operation. + Future SignInWithCredential(...); + // Get the result of the pending / last async operation for the method. + Future SignInWithCredentialLastResult(); + + Usage examples: + // call and register callback + auto& result = SignInWithCredential(); + result.set_callback([](result) { if (result == kComplete) { do_something_neat(); wake_up(); } }); + // wait + + // call and poll #1 (saving result) + auto& result = SignInWithCredential(); + while (result.value() != kComplete) { + // wait + } + + // call and poll #2 (result stored in API) + SignInWithCredential(); + while (SignInWithCredentialLastResult().value() != kComplete) { + } +``` + +### API Initiated Async Event Handling + +* Follow the + [listener / observer pattern](https://en.wikipedia.org/wiki/Observer_pattern) + for API initiated (i.e where the caller doesn't initiate the event) + async events. +* Provide a queued interface to allow users to poll for events. + +e.g. + +```c++ + class GcmListener { + public: + virtual void OnDeletedMessage() {} + virtual void OnMessageReceived(const MessageReceivedData* data) {} + }; + + class GcmListenerQueue : private GcmListener { + public: + enum EventType { + kEventTypeMessageDeleted, + kEventTypeMessageReceived, + }; + + struct Event { + EventType type; + MessageReceivedData data; // Set when type == kEventTypeMessageReceived + }; + + // Returns true when an event is retrieved from the queue. + bool PollEvent(Event *event); + }; + + // Wait for callbacks + class MyListener : public GcmListener { + public: + virtual void OnDeletedMessage() { /* do stuff */ } + virtual void OnMessageReceived() { /* display message */ } + }; + MyListener listener; + gcm::Initialize(app, &listener); + + // Poll + GcmListenerQueue queued_listener; + gcm::Initialize(app, &queued_listener); + GcmListenerQueue::Event event; + while (queued_listener(&event)) { + switch (event.type) { + case kEventTypeMessageDeleted: + // do stuff + break; + case kEventTypeMessageReceived: + // display event.data + break; + } + } +``` diff --git a/auth/src/desktop/auth_desktop.cc b/auth/src/desktop/auth_desktop.cc index dc9aca2950..e934013c64 100644 --- a/auth/src/desktop/auth_desktop.cc +++ b/auth/src/desktop/auth_desktop.cc @@ -575,6 +575,11 @@ void Auth::UseEmulator(std::string host, uint32_t port) { auth_impl->assigned_emulator_url.append(std::to_string(port)); } +AuthError Auth::UseUserAccessGroup(const char* access_group) { + // This is an iOS-only feature. No-op on other platforms. + return kAuthErrorNone; +} + void InitializeTokenRefresher(AuthData* auth_data) { auto auth_impl = static_cast(auth_data->auth_impl); auth_impl->token_refresh_thread.Initialize(auth_data); diff --git a/auth/src/include/firebase/auth.h b/auth/src/include/firebase/auth.h index f6809c4a57..7ad013e4c7 100644 --- a/auth/src/include/firebase/auth.h +++ b/auth/src/include/firebase/auth.h @@ -517,6 +517,21 @@ class Auth { /// not available on the current device. static Auth* GetAuth(App* app, InitResult* init_result_out = nullptr); + /// @brief Specifies a user access group for iCloud keychain access. + /// + /// This method is only functional on iOS. On other platforms, it is a no-op + /// and will always return `kAuthErrorNone`. + /// + /// If you are using iCloud keychain synchronization, you will need to call + /// this method to set the user access group. + /// + /// @param[in] access_group The user access group to use. Set to `nullptr` or + /// an empty string to use the default access group. + /// + /// @return `kAuthErrorNone` on success, or an AuthError code if an error + /// occurred. + AuthError UseUserAccessGroup(const char* access_group); + private: /// @cond FIREBASE_APP_INTERNAL friend class ::firebase::App; diff --git a/auth/src/ios/auth_ios.mm b/auth/src/ios/auth_ios.mm index a0292ba3b8..f449735670 100644 --- a/auth/src/ios/auth_ios.mm +++ b/auth/src/ios/auth_ios.mm @@ -608,5 +608,23 @@ void DisableTokenAutoRefresh(AuthData *auth_data) {} void InitializeTokenRefresher(AuthData *auth_data) {} void DestroyTokenRefresher(AuthData *auth_data) {} +AuthError Auth::UseUserAccessGroup(const char* access_group_str) { + if (!auth_data_) { + return kAuthErrorUninitialized; + } + NSString* access_group_ns_str = nil; + if (access_group_str != nullptr && strlen(access_group_str) > 0) { + access_group_ns_str = [NSString stringWithUTF8String:access_group_str]; + } + + NSError* error = nil; + BOOL success = [AuthImpl(auth_data_) useUserAccessGroup:access_group_ns_str error:&error]; + if (success) { + return kAuthErrorNone; + } else { + return AuthErrorFromNSError(error); + } +} + } // namespace auth } // namespace firebase diff --git a/cmake/external/leveldb.cmake b/cmake/external/leveldb.cmake index 3dd38315a8..bb6763a299 100644 --- a/cmake/external/leveldb.cmake +++ b/cmake/external/leveldb.cmake @@ -18,8 +18,10 @@ if(TARGET leveldb) return() endif() +if (DESKTOP AND MSVC) set(patch_file ${CMAKE_CURRENT_LIST_DIR}/../../scripts/git/patches/leveldb/0001-leveldb-1.23-windows-paths.patch) +endif() # This version must be kept in sync with the version in firestore.patch.txt. # If this version ever changes then make sure to update the version in diff --git a/scripts/gha/utils.py b/scripts/gha/utils.py index 0bddfc7bff..d75a516580 100644 --- a/scripts/gha/utils.py +++ b/scripts/gha/utils.py @@ -19,7 +19,7 @@ platforms. """ -import distutils.spawn +import shutil import glob import platform import shutil @@ -63,7 +63,7 @@ def run_command(cmd, capture_output=False, cwd=None, check=False, as_root=False, def is_command_installed(tool): """Check if a command is installed on the system.""" - return distutils.spawn.find_executable(tool) + return shutil.which(tool) def glob_exists(glob_path): diff --git a/scripts/print_github_reviews.py b/scripts/print_github_reviews.py index 880d3b3f9f..33bebdf14a 100755 --- a/scripts/print_github_reviews.py +++ b/scripts/print_github_reviews.py @@ -232,6 +232,12 @@ def parse_repo_url(url_string): default=None, help="Pull request number. If not provided, script attempts to find the latest open PR for the current git branch." ) + parser.add_argument( + "--branch", + type=str, + default=None, + help="Branch name to find the latest open PR for. Mutually exclusive with --pull_number. If neither --pull_number nor --branch is provided, uses the current git branch." + ) parser.add_argument( "--url", type=str, @@ -254,7 +260,7 @@ def parse_repo_url(url_string): "--token", type=str, default=os.environ.get("GITHUB_TOKEN"), - help="GitHub token. Can also be set via GITHUB_TOKEN env var." + help="GitHub token. Can also be set via GITHUB_TOKEN env var or from ~/.github_token." ) parser.add_argument( "--context-lines", @@ -289,9 +295,23 @@ def parse_repo_url(url_string): latest_line_comment_activity_dt = None processed_comments_count = 0 - if not args.token: - sys.stderr.write(f"Error: GitHub token not provided. Set GITHUB_TOKEN or use --token.{error_suffix}\n") + token = args.token + if not token: + try: + with open(os.path.expanduser("~/.github_token"), "r") as f: + token = f.read().strip() + if token: + sys.stderr.write("Using token from ~/.github_token\n") + except FileNotFoundError: + pass # File not found is fine, we'll check token next + except Exception as e: + sys.stderr.write(f"Warning: Could not read ~/.github_token: {e}\n") + + + if not token: + sys.stderr.write(f"Error: GitHub token not provided. Set GITHUB_TOKEN, use --token, or place it in ~/.github_token.{error_suffix}\n") sys.exit(1) + args.token = token # Ensure args.token is populated for the rest of the script final_owner = None final_repo = None @@ -341,26 +361,40 @@ def parse_repo_url(url_string): sys.exit(1) pull_request_number = args.pull_number - current_branch_for_pr_check = None # Store branch name if auto-detecting PR + branch_to_find_pr_for = None + + if args.pull_number and args.branch: + sys.stderr.write(f"Error: --pull_number and --branch are mutually exclusive.{error_suffix}\n") + sys.exit(1) + if not pull_request_number: - sys.stderr.write("Pull number not specified, attempting to find PR for current branch...\n") - current_branch_for_pr_check = get_current_branch_name() - if current_branch_for_pr_check: - sys.stderr.write(f"Current git branch is: {current_branch_for_pr_check}\n") - pull_request_number = get_latest_pr_for_branch(args.token, OWNER, REPO, current_branch_for_pr_check) + if args.branch: + branch_to_find_pr_for = args.branch + sys.stderr.write(f"Pull number not specified, attempting to find PR for branch: {branch_to_find_pr_for}...\n") + else: + sys.stderr.write("Pull number and branch not specified, attempting to find PR for current git branch...\n") + branch_to_find_pr_for = get_current_branch_name() + if branch_to_find_pr_for: + sys.stderr.write(f"Current git branch is: {branch_to_find_pr_for}\n") + else: + sys.stderr.write(f"Error: Could not determine current git branch. Cannot find PR automatically.{error_suffix}\n") + sys.exit(1) + + if branch_to_find_pr_for: # This will be true if args.branch was given, or if get_current_branch_name() succeeded + pull_request_number = get_latest_pr_for_branch(args.token, OWNER, REPO, branch_to_find_pr_for) if pull_request_number: - sys.stderr.write(f"Found PR #{pull_request_number} for branch {current_branch_for_pr_check}.\n") + sys.stderr.write(f"Found PR #{pull_request_number} for branch {branch_to_find_pr_for}.\n") else: - sys.stderr.write(f"No open PR found for branch {current_branch_for_pr_check} in {OWNER}/{REPO}.\n") # Informational - else: - sys.stderr.write(f"Error: Could not determine current git branch. Cannot find PR automatically.{error_suffix}\n") - sys.exit(1) + sys.stderr.write(f"No open PR found for branch {branch_to_find_pr_for} in {OWNER}/{REPO}.\n") + # If branch_to_find_pr_for is None here, it means get_current_branch_name() failed and we already exited. if not pull_request_number: # Final check for PR number - error_message = "Error: Pull request number is required." - if not args.pull_number and current_branch_for_pr_check: # Auto-detect branch ok, but no PR found - error_message = f"Error: Pull request number not specified and no open PR found for branch {current_branch_for_pr_check}." - # The case where current_branch_for_pr_check is None (git branch fail) is caught and exited above. + error_message = "Error: Pull request number could not be determined." + if args.branch: # Specific error if --branch was used + error_message = f"Error: No open PR found for specified branch '{args.branch}'." + elif not args.pull_number and branch_to_find_pr_for: # Auto-detect current branch ok, but no PR found + error_message = f"Error: Pull request number not specified and no open PR found for current branch '{branch_to_find_pr_for}'." + # The case where current_branch_for_pr_check (now branch_to_find_pr_for) is None (git branch fail) is caught and exited above. sys.stderr.write(f"{error_message}{error_suffix}\n") sys.exit(1)