Fixing argument issues with Wilbur and platform scripts #653
Conversation
This file contains hidden or 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
peer_options was always being built and passed to wilbur with the platform_scripts. This meant that if you wished to pass your own peer_options you could not use the platform_scripts. This fixes that by checking that peer_options has not already been supplied as a parameter. Additionally, there is also now a clear delimeter between script arguments and command arguments.
🦋 Changeset detectedLatest commit: 39c1013 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Similar to the linux changes this change should allow --peer_options and --http_root to be passed to the server without being stomped on by the script.
This is to make it easier to provide JSON data to the peer_options parameter without having to escape JSON data on the command line.
mcottontensor
changed the title
lukehb
reviewed
May 30, 2025
lukehb
requested changes
May 30, 2025
Co-authored-by: Luke Bermingham <1215582+lukehb@users.noreply.github.com>
mcottontensor
added
auto-backport
github-actions bot
pushed a commit
that referenced
this pull request
May 30, 2025
* fix(wilbur): Fixing issues with passing arguments to wilbur. (Linux) peer_options was always being built and passed to wilbur with the platform_scripts. This meant that if you wished to pass your own peer_options you could not use the platform_scripts. This fixes that by checking that peer_options has not already been supplied as a parameter. Additionally, there is also now a clear delimeter between script arguments and command arguments. * fix(wilbur): Fixing issues with passing arguments. (Windows) Similar to the linux changes this change should allow --peer_options and --http_root to be passed to the server without being stomped on by the script. * feat(wilbur): Adding --peer_options_file to wilburs cli arguments. This is to make it easier to provide JSON data to the peer_options parameter without having to escape JSON data on the command line. * chore(actions): Updating actions to match the new platform_script args. * fix(actions): Fixed missing comma in argument list. * fix(extras): Updating the docker commands for the new arguments. * chore(changeset): Adding changeset for wilbur. * Update file not found error message Co-authored-by: Luke Bermingham <1215582+lukehb@users.noreply.github.com> --------- Co-authored-by: Luke Bermingham <1215582+lukehb@users.noreply.github.com> (cherry picked from commit 584617b)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
mcottontensor
added a commit
that referenced
this pull request
May 30, 2025
* fix(wilbur): Fixing issues with passing arguments to wilbur. (Linux) peer_options was always being built and passed to wilbur with the platform_scripts. This meant that if you wished to pass your own peer_options you could not use the platform_scripts. This fixes that by checking that peer_options has not already been supplied as a parameter. Additionally, there is also now a clear delimeter between script arguments and command arguments. * fix(wilbur): Fixing issues with passing arguments. (Windows) Similar to the linux changes this change should allow --peer_options and --http_root to be passed to the server without being stomped on by the script. * feat(wilbur): Adding --peer_options_file to wilburs cli arguments. This is to make it easier to provide JSON data to the peer_options parameter without having to escape JSON data on the command line. * chore(actions): Updating actions to match the new platform_script args. * fix(actions): Fixed missing comma in argument list. * fix(extras): Updating the docker commands for the new arguments. * chore(changeset): Adding changeset for wilbur. * Update file not found error message --------- (cherry picked from commit 584617b) Co-authored-by: mcottontensor <80377552+mcottontensor@users.noreply.github.com> Co-authored-by: Luke Bermingham <1215582+lukehb@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Relevant components:
Problem statement:
The current platform scripts don't allow you to pass --peer_options (and possibly some other parameters) to Wilbur because the argument is constructed in the platform scripts themselves and replaces any user passed peer_options.
Solution
The platform scripts now check if any parameter it would normally pass to Wilbur already were passed by the user. If so it skips adding that parameter to the call to Wilbur.
Another change that comes with this is the implementation of an explicit delimeter between script arguments and Wilbur arguments that will be passed directly. Now anything before the
--marker is treated as a script parameter, and if an unknown parameter is found an error is produced (previously unknown parameters would be passed to Wilbur). And anything after the marker is passed directly to Wilbur. This means after this update, users will need to properly separate script parameters and Wilbur parameters with--.One final change with this PR is that there is a new
--peer_options_fileparameter for Wilbur. JSON is notoriously bad to pass as a command line argument value due to formatting, escaping, spaces etc. It does not parse well on the command line. To aleviate this, I have added the new--peer_options_fileparameter that allows users to provide a JSON file for the peer options. This file is read, parsed and passed in as the peer options for the signalling server..Fixes #650
Documentation