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

Remove run.architecture #262

Closed
michaelcfanning opened this issue Oct 11, 2018 · 3 comments
Closed

Remove run.architecture #262

michaelcfanning opened this issue Oct 11, 2018 · 3 comments

Comments

@michaelcfanning
Copy link
Contributor

Do we even require this? Should people encode architecture as part of run.instanceId (which contains a logical identifier). If architecture is not encoded in the run.instanceId, then we must be comfortable with the architecture serving implicitly to determine the equivalence class of runs. i.e., the analysis results associated with the x64 build of a product are not the same as the analysis results of the x86 build.

We should probably zap this property. I note that we don't have other build qualifiers, such as debug vs. retail.

@ghost
Copy link

ghost commented Oct 11, 2018

[‎10/‎11/‎2018 4:06 PM] Michael Fanning:
see #262
I think we should zap run.architecture
or find a way to get it onto runAutomationDetails

[‎10/‎11/‎2018 4:07 PM] Larry
Interesting. I don't know why "architecture" became a first-class citizen. Could I ask you to search the Issues for "architecture" and see if there was a rationale?
I don't think it belongs in runAutomationDetails for the same reason I don't think "buildFlavor" (Debug/Release/Instrumented/whatever) does.

[‎10/‎11/‎2018 4:08 PM] Michael Fanning:
the rationale is as described in the bug
not strictly true

[‎10/‎11/‎2018 4:09 PM] Larry
I meant the rationale for defining run.architecture in the first place.
To finish my previous thought...

[‎10/‎11/‎2018 4:09 PM] Michael Fanning:
an analysis run in many cases is tied very specifically to the architecture to which the build is targeted

[‎10/‎11/‎2018 4:09 PM] Larry
... the engineering system should be allowed to categorize runs however it wants.

[‎10/‎11/‎2018 4:09 PM] Michael Fanning:
x64 prefast results are not the same as the x86, even for the same product
similarly debug build scan results are not the same as retail: the code emit is different and this produces different results
you cannot results match between a release and debug analysis for most tools

[‎10/‎11/‎2018 4:10 PM] Larry
Right. So my point is:

[‎10/‎11/‎2018 4:10 PM] Michael Fanning:
so it is relevant to the automation details because this data identifies an equivalence class of runs

[‎10/‎11/‎2018 4:10 PM] Larry
There is nothing special about "architecture" in this regard. Many build settings affect code emit.

[‎10/‎11/‎2018 4:10 PM] Michael Fanning:
but we already have an instanceId
so we should recommend that people encode this there

[‎10/‎11/‎2018 4:10 PM] Larry
Yes!

[‎10/‎11/‎2018 4:11 PM] Michael Fanning:
if they have a need to distinguish these equivalence classes
yes

[‎10/‎11/‎2018 4:12 PM] Larry
Yes, we should encourage engineering systems to encode settings that affect code emit and that might vary from build to build (arch, flavor, ... others?) into the instanceId hierarchical string.
Having said that...
... I would still want to paw back through the issues and find out why we felt "architecture" deserved first-class status.
(end)

[‎10/‎11/‎2018 4:13 PM] Michael Fanning:
you must do this yourself
i understand clearly the rationale based on my domain knowledge
:)
i remember talking about this

[‎10/‎11/‎2018 4:14 PM] Larry
I can accept that you remember the history, and that's fine. But then why did we not invent run.buildFlavor at the same time?

[‎10/‎11/‎2018 4:14 PM] Michael Fanning:
the spec is very clear and backs me up

A run object MAY contain a property named architecture whose value is a string that specifies the hardware architecture at which the analysis targets are targeted. This does not need to be the same as the architecture on which the analysis tool is executed.
This specification does not specify a set of valid values for the ...

looks like an oversight or inconsistency

[‎10/‎11/‎2018 4:14 PM] Larry
perfect. that's all I needed.
If you believe that "flavor" should have had the same status, but we overlooked it, I agree with removing run.architecture and providing clear guidance for instanceId.

[‎10/‎11/‎2018 4:15 PM] Michael Fanning:
People could construct a logical id that has these concepts:
Nightly FxCop Run for SARF-SDK/x86/Release/10-10-2018

[‎10/‎11/‎2018 4:16 PM] Larry
Yes, that is essentially the example I put in the spec.

[‎10/‎11/‎2018 4:16 PM] Michael Fanning:
you have to make a choice on nesting...
hierarchy
yep

[‎10/‎11/‎2018 4:17 PM] Larry
I think the first and last components (run type, date) are clear. I don't think there's much to choose in how you order the middle components.

[‎10/‎11/‎2018 4:17 PM] Michael Fanning:
yes

@ghost ghost changed the title Should run.architecture move to runAutomationDetails? Remove run.architecture Oct 23, 2018
@ghost
Copy link

ghost commented Oct 23, 2018

Renamed the issue. We've decided that architecture can be an element of the hierarchical instanceId, but it doesn't have to be; it's not a first-class citizen. The spec already shows examples where it appears as a hierarchical component.

@ghost ghost self-assigned this Oct 23, 2018
@ghost ghost added impact-breaks-producers 2.1.0-CSD.1 Will be fixed in SARIF v2.1.0 CSD.1. design-improvement change-draft-superfluous The change is a simple rename; no change draft needed. labels Oct 23, 2018
ghost pushed a commit that referenced this issue Oct 24, 2018
@ghost ghost added resolved-fixed and removed change-draft-superfluous The change is a simple rename; no change draft needed. labels Oct 24, 2018
@ghost
Copy link

ghost commented Oct 24, 2018

Changes made directly in provisional draft.

@ghost ghost closed this as completed Oct 24, 2018
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant