Skip to content

Remove redundant "-:NOREFS" from SymStore cli options#13439

Merged
seanbudd merged 6 commits into
masterfrom
removeNoRefs
Jun 17, 2022
Merged

Remove redundant "-:NOREFS" from SymStore cli options#13439
seanbudd merged 6 commits into
masterfrom
removeNoRefs

Conversation

@seanbudd

@seanbudd seanbudd commented Mar 8, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Link to issue number:

Closes: #12912

Summary of the issue:

As per the documentation -:NOREFS should be used with /p. Reference pointer files are not included unless we use the /p flag.

Description of how this pull request fixes the issue:

Removes -:NOREFS and reorders options to be inline with documentation.

Testing strategy:

Removing -:NOREFS might raise the concern that new reference pointer files will be included in the symbols.zip symbols store, which is created when building NVDA.
The symbols.zip of a build from this branch was diffed with the symbols.zip of alpha that this is branched from.
Diffing further shows that only the end component (hash) of the file has changed.
This means that no new files are included or excluded, only the file contents have changed.

diff -qr "symbols (4)" "symbols (2)" | grep -ve "^Files .* differ$" | sort
Only in symbols (2)/symbols/IAccessible2proxy.dll.pdb: C31A420BF0CB4DF590D828FD379612DD1
Only in symbols (2)/symbols/IAccessible2proxy.dll.pdb: F03568EF25004A2F9272E013D8CE537B1
Only in symbols (2)/symbols/IAccessible2proxy.dll: 6226A9D91d000
Only in symbols (2)/symbols/IAccessible2proxy.dll: 6226AB3126000
Only in symbols (2)/symbols/ISimpleDOM.dll.pdb: 8E7CED0C52ED4C6CAA30F7BC4DDD8E4B1
Only in symbols (2)/symbols/ISimpleDOM.dll.pdb: E36CF2F96305442181C17FF576D3E7101
Only in symbols (2)/symbols/ISimpleDOM.dll: 6226A9DB1a000
Only in symbols (2)/symbols/ISimpleDOM.dll: 6226AB3321000
Only in symbols (2)/symbols/Microsoft.UI.UIAutomation.dll: 6226AAC28b000
Only in symbols (2)/symbols/UIARemote.dll.pdb: 7CE24A050719406CB4703AB63411AA101
Only in symbols (2)/symbols/UIARemote.dll: 6226AB2E30000
Only in symbols (2)/symbols/espeak.dll.pdb: 88188917489A4151AA097CF1234C90241
Only in symbols (2)/symbols/espeak.dll: 6226ABACd8000
Only in symbols (2)/symbols/liblouis.dll.pdb: A0D8765570374ABE8689F5459E0D80B81
Only in symbols (2)/symbols/liblouis.dll: 6226AB9D62000
Only in symbols (2)/symbols/nvdaHelperLocal.dll.pdb: A89C9D4FE34F42C485B819BE8E90FFCC1
Only in symbols (2)/symbols/nvdaHelperLocal.dll: 6226AAD64d000
Only in symbols (2)/symbols/nvdaHelperLocalWin10.dll.pdb: 11F3F881A9D94EBC845791E1C1BC4AE31
Only in symbols (2)/symbols/nvdaHelperLocalWin10.dll: 6226AAE394000
Only in symbols (2)/symbols/nvdaHelperRemote.dll.pdb: 3D59947061724082B82131D5887C4BBC1
Only in symbols (2)/symbols/nvdaHelperRemote.dll.pdb: B6802911C46C4163AFC33D3DE2B04C5A1
Only in symbols (2)/symbols/nvdaHelperRemote.dll: 6226AB10fa000
Only in symbols (2)/symbols/nvdaHelperRemote.dll: 6226AB6113b000
Only in symbols (2)/symbols/nvdaHelperRemoteLoader.exe.pdb: DC6AEC43F8FD45AAB7594C9B7BF94CA31
Only in symbols (2)/symbols/nvdaHelperRemoteLoader.exe: 6226AB6321000
Only in symbols (4)/symbols/IAccessible2proxy.dll.pdb: 0B51D6E7A8124FADAEC2D16B3EE341341
Only in symbols (4)/symbols/IAccessible2proxy.dll.pdb: A4813E497EA04E26BA3FF35CC89126FF1
Only in symbols (4)/symbols/IAccessible2proxy.dll: 62269C331d000
Only in symbols (4)/symbols/IAccessible2proxy.dll: 62269D9226000
Only in symbols (4)/symbols/ISimpleDOM.dll.pdb: 87912C1870AA44A3836B0774ED53BBA01
Only in symbols (4)/symbols/ISimpleDOM.dll.pdb: EB6EFEB3F4DB4E88B45E776C778829A21
Only in symbols (4)/symbols/ISimpleDOM.dll: 62269C351a000
Only in symbols (4)/symbols/ISimpleDOM.dll: 62269D9521000
Only in symbols (4)/symbols/Microsoft.UI.UIAutomation.dll: 62269D1D8b000
Only in symbols (4)/symbols/UIARemote.dll.pdb: BEC013716E28440A92DF7AC8A478B4F61
Only in symbols (4)/symbols/UIARemote.dll: 62269D8F30000
Only in symbols (4)/symbols/espeak.dll.pdb: 69CA552658544B5ABD6D44C50A339CD21
Only in symbols (4)/symbols/espeak.dll: 62269E11d8000
Only in symbols (4)/symbols/liblouis.dll.pdb: AD8A1F17756D47AE92D0915540FE656A1
Only in symbols (4)/symbols/liblouis.dll: 62269E0262000
Only in symbols (4)/symbols/nvdaHelperLocal.dll.pdb: 3FF83B49AE2B488488E63F2B11CBB5E21
Only in symbols (4)/symbols/nvdaHelperLocal.dll: 62269D324d000
Only in symbols (4)/symbols/nvdaHelperLocalWin10.dll.pdb: EF789EF0F7D944EB8D13524528A40F9F1
Only in symbols (4)/symbols/nvdaHelperLocalWin10.dll: 62269D4194000
Only in symbols (4)/symbols/nvdaHelperRemote.dll.pdb: 9178D70CE50044DE8AF7B8E651C246F81
Only in symbols (4)/symbols/nvdaHelperRemote.dll.pdb: AC016236157844CA9BB5B1B659C29ABB1
Only in symbols (4)/symbols/nvdaHelperRemote.dll: 62269D70fa000
Only in symbols (4)/symbols/nvdaHelperRemote.dll: 62269DC413b000
Only in symbols (4)/symbols/nvdaHelperRemoteLoader.exe.pdb: F76BC507A80D40C298AF5757309998111
Only in symbols (4)/symbols/nvdaHelperRemoteLoader.exe: 62269DC621000

Known issues with pull request:

n/a

Change log entries:

n/a

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English

@seanbudd seanbudd requested a review from a team as a code owner March 8, 2022 01:54
@seanbudd seanbudd requested a review from feerrenrut March 8, 2022 01:54
feerrenrut
feerrenrut reviewed Mar 9, 2022
View reviewed changes

@feerrenrut feerrenrut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code change looks fine, confirmed order matches docs, and option requirements are as described.

I'm not sure I understand what conclusions to draw from the testing section of the description:

  • Wouldn't it be more clear to test this same branch before and after this buildSymbolStore.ps1 change?
  • I assume the intention is to show that there is no difference in the symbol store output due to this change?

@seanbudd

seanbudd commented Mar 9, 2022

Copy link
Copy Markdown
Member Author

Wouldn't it be more clear to test this same branch before and after this buildSymbolStore.ps1 change?

I figured it was more clear to specify alpha. The branch before and after the change is the same difference as alpha/this branch.

I assume the intention is to show that there is no difference in the symbol store output due to this change?

The intention is to show that there an no new files excluded or included, the contents of the files will differ.

@seanbudd

seanbudd commented Mar 9, 2022

Copy link
Copy Markdown
Member Author

I've updated the PR description to make this more clear.

@feerrenrut

feerrenrut commented Mar 10, 2022

Copy link
Copy Markdown
Contributor

The intention is to show that there an no new files excluded or included, the contents of the files will differ.

This implies that the command doesn't alter the contents of these files?
If it does alter the content, we should have an explanation of what is changing / why.

These files are created based on the NVDA source files, if the source changes the pdb files change, using different source ensures the contents will differ. Using identical source files will help us isolate other causes of change.

If we can show that the symbol store is binary equal before and after this change to params, then there is no risk of breaking the symbol store.

Rockstar50373
Rockstar50373 approved these changes Mar 10, 2022
View reviewed changes

@Rockstar50373 Rockstar50373 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

#13433

@feerrenrut feerrenrut added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Mar 18, 2022
@seanbudd seanbudd marked this pull request as draft March 21, 2022 02:52
@seanbudd

seanbudd commented Jun 17, 2022

Copy link
Copy Markdown
Member Author

@feerrenrut Note that 2 different builds of the same source code will yield different binaries.
Hashing symbols.zip after pushing an empty commit creates a different hash.

2 different builds with the exact same source code also yield the same output to what is described in the PR description, using diff -qr.

@seanbudd seanbudd marked this pull request as ready for review June 17, 2022 04:35
@seanbudd seanbudd requested a review from feerrenrut June 17, 2022 04:35
@seanbudd

seanbudd commented Jun 17, 2022

Copy link
Copy Markdown
Member Author

Manually diffing these files shows the majority of differences are timestamps and random strings being changed.
Considering Microsoft has explicitly suggested to remove this option, as it is redundant, and we are not following the documentation, it is not worth investigating the difference between these files. The file structure remains the same.

@seanbudd seanbudd merged commit c75bc4d into master Jun 17, 2022
@seanbudd seanbudd deleted the removeNoRefs branch June 17, 2022 07:36
@nvaccessAuto nvaccessAuto added this to the 2022.3 milestone Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@feerrenrut feerrenrut feerrenrut approved these changes

@Rockstar50373 Rockstar50373 Rockstar50373 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.

Projects

None yet

Milestone

2022.3

Development

Successfully merging this pull request may close these issues.

Update buildSymbolStore.ps1 symstore.exe usage to be in-line with documentation.

4 participants

@seanbudd @feerrenrut @Rockstar50373 @nvaccessAuto