Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add inferred runtime identifiers #7141

Closed
wants to merge 5 commits into from

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Mar 23, 2021

During source build it's desirable to add RIDs to the runtime graph
without actually. Making a change to sources.

For example: the tar-ball is produced and then built on some new distro.

The added RID should follow the compatibility rules for existing RIDs
so we parse it and try to find the most appropriate RuntimeGroup to add
it to.

cc @tmds @omajid

ericstj added 2 commits March 23, 2021 11:13
During source build it's desirable to add RIDs to the runtime graph
without actually. Making a change to sources.

For example: the tar-ball is produced and then built on some new distro.

The added RID should follow the compatibility rules for existing RIDs
so we parse it and try to find the most appropriate RuntimeGroup to add
it to.
@ericstj ericstj requested review from ViktorHofer and Anipik March 23, 2021 18:41
char current = runtimeIdentifier[i];
partLength = i - partStart;

switch (parseState)
Copy link
Member

Choose a reason for hiding this comment

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

Did you consider using a regular expression for parsing instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I had a regex and it was much more difficult to read once it was complicated enough to handle all cases and capture all fields that we needed. This simple state machine is easier to read and debug (and happens to be more efficient though that's not a real concern for this one shot task).

@ViktorHofer
Copy link
Member

Does anyone except dotnet/runtime mutate the runtime graph? Would dotnet/runtime be a better place for the runtime graph code to live?

@ericstj
Copy link
Member Author

ericstj commented Mar 23, 2021

Does anyone except dotnet/runtime mutate the runtime graph? Would dotnet/runtime be a better place for the runtime graph code to live?

This has a single consumer in dotnet\runtime: The platforms package. If there was a good place in dotnet/runtime to both build and test tasks we could move it, though I'm not seeing a ton of value in doing so.. At the time this was written that wasn't the case. I don't think we should block the addition of this feature on moving this out of arcade. Perhaps when we want to remove the pkgproj stuff we can move this to dotnet/runtime.

ericstj added 2 commits March 23, 2021 13:35
Versions may contain insignificant whitespace which must be preserved.
@ericstj
Copy link
Member Author

ericstj commented Mar 24, 2021

Note to reviewers: best to review all changes after first refactoring commit (checkout GitHub's shift-click feature to reduce the commit range).

@ViktorHofer
Copy link
Member

This has a single consumer in dotnet\runtime: The platforms package. If there was a good place in dotnet/runtime to both build and test tasks we could move it, though I'm not seeing a ton of value in doing so.. At the time this was written that wasn't the case. I don't think we should block the addition of this feature on moving this out of arcade. Perhaps when we want to remove the pkgproj stuff we can move this to dotnet/runtime.

Unrelated to this PR but let me respond. If there's only a single consumer then the code shouldn't live in Arcade. There is infrastructure in dotnet/runtime that allows to build tasks, test and consume them.

I don't think we should block the addition of this feature on moving this out of arcade.

Neither do I. It was a side comment unrelated to this PR as I wasn't aware that this code exists in the packaging code.

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

Scanned the code but didn't test the state machine carefully. I trust that your tests will cover most cases. Code coverage numbers would be interesting here though.

@ericstj
Copy link
Member Author

ericstj commented Mar 30, 2021

Marking no-merge as I plan to move this to dotnet/runtime

@ericstj
Copy link
Member Author

ericstj commented Apr 2, 2021

We're moving this code to dotnet/runtime

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

Successfully merging this pull request may close these issues.

2 participants