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 script for checking symbol availability #2492

Merged
merged 4 commits into from
Apr 15, 2019
Merged

Conversation

JohnTortugo
Copy link
Contributor

@JohnTortugo JohnTortugo commented Apr 10, 2019

Closes: #1874

It was decided by e-mail that we would use dotnet symbol tool to check availability & completeness of symbols for packages.

The tool however doesn't accept a package for checking availability, only individual modules. The script in this PR opens packages and check for availability of symbols for each contained module using dotnet symbol tool.

This script is a temporary solution. Once the maintainer of dotnet symbol tool (@mikem8361) returns from vacation we'll work on improving the tool to accept a package as input for validation.

It was also agreed that for now symbol completeness will be done as part of symbol availability. Again, until dotnet symbol tool can be patched to do the right thing for completeness.

@JohnTortugo JohnTortugo self-assigned this Apr 10, 2019
Copy link
Member

@dagood dagood left a comment

Choose a reason for hiding this comment

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

Any tests for this? I'd get a much better understanding of what's expected to work if there were some.

I'm also curious about using powershell and this being in eng/common. Is this just a normal design for pipeline things (which are also supposed to run in CI per an email thread)?

eng/common/CheckSymbols.ps1 Outdated Show resolved Hide resolved
eng/common/CheckSymbols.ps1 Outdated Show resolved Hide resolved
eng/common/CheckSymbols.ps1 Outdated Show resolved Hide resolved
eng/common/CheckSymbols.ps1 Outdated Show resolved Hide resolved
eng/common/CheckSymbols.ps1 Outdated Show resolved Hide resolved
eng/common/CheckSymbols.ps1 Outdated Show resolved Hide resolved
eng/common/CheckSymbols.ps1 Outdated Show resolved Hide resolved
eng/common/CheckSymbols.ps1 Outdated Show resolved Hide resolved
eng/common/CheckSymbols.ps1 Outdated Show resolved Hide resolved
@jcagme
Copy link
Contributor

jcagme commented Apr 10, 2019

Is the plan of creating this on eng/common to copy it the artifacts folder which then the release definition can access? I think, eventually, we need to start investigating how to change this model so repos don't get more things that are not useful for them. Maybe analyze what we talked about of cloning arcade and get everything from there... I think for now this location is fine, we can cleanup after the other work is implemented.

Copy link
Contributor

@jcagme jcagme left a comment

Choose a reason for hiding this comment

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

LGTM after all @dagood's comments have been addressed

@tmat
Copy link
Member

tmat commented Apr 10, 2019

Is the plan of creating this on eng/common to copy it the artifacts folder which then the release definition can access? I think, eventually, we need to start investigating how to change this model so repos don't get more things that are not useful for them.

Yes, we definitely need to change this. I'd suggest we move all assets that are used in the publishing pipeline to a package. Perhaps to Arcade SDK package under directory Publishing or something like that.

@JohnTortugo
Copy link
Contributor Author

Any tests for this? I'd get a much better understanding of what's expected to work if there were some.

I've done a bunch of tests. Here is an instance. The first stage runs the PS from this PR and finds a few modules are missing symbols. Second stage publish symbols for those packages. Third stage again runs the PS from this PR and now succeeded because the symbols were published.

@dagood
Copy link
Member

dagood commented Apr 10, 2019

That's good to see, I meant automated tests though.

@JohnTortugo
Copy link
Contributor Author

Yes, we definitely need to change this. I'd suggest we move all assets that are used in the publishing pipeline to a package. Perhaps to Arcade SDK package under directory Publishing or something like that.

That's definitely going to happen. I just got other things on my plate. I think next week I should be able to get that work done.

eng/common/CheckSymbols.ps1 Outdated Show resolved Hide resolved
}
elseif (Test-Path $DylibDwarf)
{
return "Dward for Dylib"
Copy link
Member

Choose a reason for hiding this comment

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

Typo, "Dward"

But on that note, I think it would make more sense to reduce the scope of this script, maybe only check for PDB for the proof of concept:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be better to delete the current check?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure what you're comparing against for "better"... my suggestion is to delete checks including this current one.

On the other hand, no need to block on this, it's already written and everything will naturally get fixed by being replaced by built-in checks either way.

eng/common/CheckSymbols.ps1 Outdated Show resolved Hide resolved
eng/common/CheckSymbols.ps1 Show resolved Hide resolved
@JohnTortugo
Copy link
Contributor Author

I'd let to merge this by tomorrow. Is there anything else I should address?

@JohnTortugo JohnTortugo merged commit 06c13cd into master Apr 15, 2019
@JohnTortugo JohnTortugo deleted the SymbolAvailability branch April 15, 2019 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automate validation of symbols
5 participants