fix: improve error handling for unsupported OS #91
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
See unhelpful error message in #40 for unsupported OS
|
@EstebanBorai if you're happy with this one, and tests pass, this is ready to be merged now. Then, the final step: unit tests! There may be some slight configurations I need you to do on your end to give Cirrus access to this repository, but I'll let you know once I start work on it. |
In the previous commit, we only conditionally imported std::env if the operating system required it. I wanted to be more explicit with this (opt-in rather than opt-out), so we did so in this commit.
in 204518e, I refactored some imports to reduce code duplication, but in doing so I accidentally removed a necessary import for Windows in calculation of local IP. Readding this back in
|
Apologies for a few failed pipelines! It's difficult to check changes locally when you don't have all of the operating systems at your disposal. Thank god for tests! They all seem to be passing now, so feel free to merge if you're happy with the diff. |
Nothing to worry about! And thanks for sure! |
EstebanBorai
changed the title
EstebanBorai
added a commit
that referenced
this pull request
Nov 29, 2022
* feat: OpenBSD support (#85) * Add OpenBSD support OpenBSD can use the same API as MacOS and thus be supported. * Update src/lib.rs Co-authored-by: Jake Ireland <[email protected]> * Update src/lib.rs Co-authored-by: Jake Ireland <[email protected]> * Update src/lib.rs Co-authored-by: Jake Ireland <[email protected]> Co-authored-by: Esteban Borai <[email protected]> Co-authored-by: Jake Ireland <[email protected]> * Refactor and improve documentation for BSD support (#87) * Rename src/macos.rs -> src/bsd.rs * Expand support to all BSD-based; update docs accordingly Previously the compiler flag `target_os` was only implemented for OpenBSD. Here, we add support also for FreeBSD, NetBSD, and Dragonfly (another BSD-based OS). While we may not be able to reasonably test all OSes, these are all BSD-based and hence should all work as it does on macOS and FreeBSD. * Fix formatting for BSD-based config attributes * Add note on testing on FreeBSD on behalf of other BSD systems See relevant discussion: - #87 (comment) - #87 (comment) * fix: improve error handling for unsupported OS (#91) * Implement CI workflow for FreeBSD using Cirrus (#92) * Implement initial Cirrus CI workflow for FreeBSD There may still be remaining tests and refinements to this, but this is the initial state of testing on BSD. * Add minimum supported Rust version to Cirrus CI config * Add version checker to setup in Cirrus CI * Modify setup script in Cirrus CI to use fetch rather than curl * Use FreeBSD 13 for BSD unit tests GitHub workflows for this project use "latest" versions of the operating systems they test on. While FreeBSD images within Cirrus do not have a "latest" option, the latest stable release is 13.1, so we test on this image https://cirrus-ci.org/guide/FreeBSD/ * Fix typo in "cargo" in Cirrus CI config * Use explicit commands rather than environment variables in Cirrus For some reason, the environment variables were being set in the top and could be used in top-level nodes, but they were failing in matrix tests: https:/jakewilliami/local-ip-address/runs/9611248995 This commit expands all of these out into explicit commmands, hence doing away with most variables. I have kept TOOLCHAIN defined as the MSRV in the hope that this works. * Remove Rust component addition from setup in Cirrus This Rust component (Clippy) is set up for the Clippy test, so it does not need to be in the common setup script. * Add env setup to individual tasks in Cirrus BSD matrix Env still not being set properly, trialling individual setup command to put rustup and cargo in the path * Use individual tasks over one task with matrix in Cirrus CI One question I had when researching Cirrus CI for our use case is whether it is better to have individual tasks which run each separate test (but thus require to download and build Rust for each test), or to have one task which builds Rust only once, and can then run subtasks using the matrix keyword. While I had not yet found a consensus in my research, it seems The Gods have decided for us, as the environment being set up at the top level of the Cirrus CI config did not allow the subtasks to have the same set up. This commit hopefully fixes this issue, but using individual tasks over one task with a matrix. It will, however, download and build Rust for each test. * Add env setup to test script in Cirrus CI * Disable warnings in build and test in Cirrus CI Many of the CI workflows from which I gained inspiraton in creation of this one use the "-D warnings" RUSTFLAGS environment parameter. I believe this is to ensure clean output in CI. It seems to be good practice to do it this way (in CI): https://www.reddit.com/r/rust/comments/f5xpib/comment/fi1k83a/ * Correct Cargo toolchain version specification in Cirrus CI config * Source path from cargo env for install scripts in Cirrus CI * Remove Rustup install script after setup in Cirrus workflow * Remove release specification from tasks The target specification was an artefact obtained from one of my source references for running CI tests on Cirrus for Rust, however I cannot determine where the $TARGET variable is set, and it is causing jobs to fail, so I am removing this specification to make these tests more in line with Esteban's GitHub workflow tests * Correct off-by-one error in parsing of getifaddrs for BSD We previously used a while loop with a condition to iterate over the results of libc::gerifaddrs, however as we checked if ifa.ifa_next was null from the start, we were not checking one network interface. This caused somme bugs in tests for FreeBSD, which we partially fix in the present commit (there is another bug that will be fixed in the next commit, involving a hard-coded search for specific interface names). * Search for epair0b as well as en0 interfaces from FreeBSD FreeBSD tests were failing due to no en0 interfaces. In this commit, we modify the find_ifa function: this method now is called find_ifas, and has a new type signature, in which it accepts a vector of possible interface names on which to match. For future, it would be ideal to dynamically determine whether the interface without hard-coding interface names. * Run ifconfig in setup for FreeBSD tests * Dynamically obtain local IP for BSD (#95) Co-authored-by: Denis Fondras <[email protected]> Co-authored-by: Jake Ireland <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This MR aims to improve handling of unsupported OS by handling error cases in the docstrings, as well as implementing fall-back functions in conditional compilation for unsupported operating systems.
See unhelpful error message in #40 for unsupported OS.
Initial suggestion for this feature mentioned here: #85 (comment).