Update the SLASH_ALVEO flow for SLASH's renamed slashkit linker - #1630
Merged
Conversation
Contributor
Author
|
E2E test was successful (SLASH_ALVEO build of the cybersecurity MLP to a |
merkelmarrow
marked this pull request as ready for review
July 16, 2026 14:07
Contributor
Author
|
Want to improve the tests. |
merkelmarrow
marked this pull request as draft
July 16, 2026 17:37
Move the SLASH linker argv assembly out of SlashLink.apply into a pure _slash_link_command helper. Behaviour is unchanged: the command still invokes the linker binary passed by the caller. This isolates the invocation so the linker command can be tested without a full dataflow model. Signed-off-by: Marco Blackwell <mblackwe@amd.com>
SLASH renamed its v80++ linker to slashkit with no alias, so SlashLink
could no longer find the binary. Resolve through resolve_xilinx_tool
("slashkit") so FINN_TOOL_DIR_OVERRIDE shims apply. Add util tests for
argv construction and for resolver integration via a monkeypatched shim
path.
Signed-off-by: Marco Blackwell <mblackwe@amd.com>
The SLASH linker deb (slashkit) declares apt dependencies (python3-jinja2 and others) that the FINN image does not already install, so apt must fetch them from the package repositories when the local deb is installed. Run apt-get update first so this does not fail against stale package lists in a cached image layer. Signed-off-by: Marco Blackwell <mblackwe@amd.com>
The USERNAME build arg names the account created with useradd, but the final USER directive used the literal USERNAME instead of $USERNAME, so the image's default user is an account that does not exist. Use $USERNAME so images layered FROM this one run as the created user. Signed-off-by: Marco Blackwell <mblackwe@amd.com>
SLASH renamed its linker to slashkit, so the packaging plumbing that still named v80++ was misleading and pointed users at a package that no longer exists. Rename the V80PP_DEB_PACKAGE environment variable to SLASHKIT_DEB_PACKAGE, the copied deb from v80pp.deb to slashkit.deb, and the build arg, comment, prompt, and getting_started docs to match. This is a breaking change for anyone setting the old variable. There is no compatibility fallback. See getting_started.rst for the renamed variable. Signed-off-by: Marco Blackwell <mblackwe@amd.com>
merkelmarrow
force-pushed
the
fix/slash-linker-rename
branch
from
July 27, 2026 20:17
224ac8a to
24da3dd
Compare
Contributor
Author
|
Force push:
|
merkelmarrow
marked this pull request as ready for review
July 27, 2026 20:26
Collaborator
@merkelmarrow This is a bug we're actively working to fix - see Xilinx/SLASH#169 - perhaps just disable the test until the fix is merged. |
Contributor
Author
The xfailing test was just an awkward reminder to add the feature later, thanks for fixing. I'll fold in the clock-hz arg for SlashLink in FINN later |
Signed-off-by: Marco Blackwell <mblackwe@amd.com>
auphelia
approved these changes
Jul 29, 2026
auphelia
left a comment
auphelia
left a comment
Collaborator
There was a problem hiding this comment.
Thank you @merkelmarrow !
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
The V80 SLASH_ALVEO backend invokes the SLASH linker as "v80++". The current SLASH release (v1.0) renamed that tool to "slashkit", so SlashLink fails to link. This PR updates every instance of v80++ to slashkit and wires it through the heavy tool resolver (
resolve_xilinx_tool).This is a breaking change for:
FINN_TOOL_DIR_OVERRIDE). A new slashkit shim needs to be added to the tool dir overrides, all slashkit invocations by FINN will fail in such a CI setup until this is done.Two related dockerfile issues were also fixed:
apt-get update, standard best practice and slashkit declares apt dependencies such as python3-jinja2 that the image doesn't already install, so the local deb install needs current package lists (would fail against stale lists cached in an earlier layer).USERreferences the literalUSERNAMEinstead of$USERNAMEso the image default user was an account that doesn't exist (this affects any image that is layered from FINN's). This was fixed.Testing
tests/fpgadataflow/test_slash_link.pythat test command construction for hw/sim, kernel list assembly.