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

Enable SourceLink #41

Merged
merged 3 commits into from
Feb 3, 2022
Merged

Enable SourceLink #41

merged 3 commits into from
Feb 3, 2022

Conversation

Irubataru
Copy link

SourceLink is quite useful for developing dependent applications as it enables users to step into the source code and open definitions of SimSharp through nuget packages. E.g. when opening the definition to a SimSharp class in a dependent project one would then get the code from github instead of getting the decompiled version from the IDE.

I am still quite new to nugets and SourceLink, but I added similar changes that the Newtonsoft repo did when adding this feature (see this pull request).

@Irubataru
Copy link
Author

I am not entirely sure why the unit test fails, it doesn't on my computer. The new warnings seem to stem from compiling an older version of the standard with a newer compiler.

@abeham
Copy link
Member

abeham commented Feb 2, 2022

Ok thanks, the test can fail when the processes is suspended for a longer time. It's not the best test, but it's a bit difficult to test parallel processes and giving some guarantee on execution time. I restarted the build. Seems to be working.

@abeham
Copy link
Member

abeham commented Feb 3, 2022

Now, I also believe the reason why upgrading from netcoreapp2.1 to net5 failed on appveyor recently. I didn't remember that the build image was declared in the appveyor config 🤦‍♂️. I think we can upgrade to net5 the sample, benchmark, and test projects now (the library should remain net45 and netstandard2.0).

Anyway, I also have only very little experience with nuget and deployment related issues. But I did a little bit of research. This source mentions that we shouldn't add ContinuousIntegrationBuild (deterministic builds) unconditionally:

These should not be enabled during local dev or the debugger won’t be able to find the local source files.

We should do ContinuousIntegrationBuild on appveyor only and then also configure appveyor so that the artifact can be downloaded and be uploaded to nuget for distribution (I was publishing local builds so far). Do you happen to know the condition that we need to check when it's running on appveyor?

@Irubataru
Copy link
Author

Irubataru commented Feb 3, 2022

Good catch. From the appveyor docs it seems like the flag would be APPVEYOR (or simply CI). I'll add it to the config.

@Irubataru
Copy link
Author

Maybe it would be a good idea to make the realtime unit tests not run on Appveyor. Could e.g. be a global config that checks the APPVEYOR environment flag and skips the test if it is set, using e.g. Xunit.SkippableFact. Can either define something in the PropertyGroup or just have a simple static method.

@abeham abeham merged commit bc25b70 into heal-research:master Feb 3, 2022
@Irubataru Irubataru deleted the feature/sourcelink branch February 3, 2022 11:54
@Irubataru
Copy link
Author

Would it be possible to publish an updated nuget package so I could see if source link works?

@abeham
Copy link
Member

abeham commented Feb 4, 2022

I uploaded it to nuget as prerelease version: https://www.nuget.org/packages/SimSharp/3.4.1-pre1

Also, the build server will from now on produce the nupkg as artifacts (retention period is one month I think): https://ci.appveyor.com/project/abeham/simsharp/build/artifacts

Can you also check the experience of debugging models? E.g. stepping through a process in the debugger? I hope that if you say "Just My Code" it will overstep the Sim# library stuff. Because for me that makes a huge difference. If I hit F10 and the debugger shows me the next line of model code, then that's good. But if your thrown into the Process class and Step() logic then stepping through processes with the debugger is annoying. I am not sure how VS treats this when the pdb is present.

@Irubataru
Copy link
Author

I am having a bit of trouble getting source link to work in VS all together. However, it works well in Rider (which is the C# IDE I actually use). I can't test the Just My Code settings yet.

However, I have two observations. First it seems like the symbol package wasn't uploaded to nuget.org. If you look at e.g. the Newtonsoft.Json page there is a link to download symbols that is not present for SimSharp. Second, there seems to be an issue with the config still. If you look at the checks in the Nuget Package Explorer there are warnings about untracked sources, so maybe we also need the <EmbedUntrackedSources>true</EmbedUntrackedSources> flag in the project file. Also, it seems like <AllowedOutputExtensionsInPackageBuildOutputFolder> might not be necessary if you publish the symbols file to nuget (see this comment).

@abeham
Copy link
Member

abeham commented Feb 4, 2022

Okay, I have no idea how this all works, but Newtonsoft.Json produces a .snupkg file and not a symbols.nupkg, because it seems like the .snupkg is an add-on to the regular .nupkg while the .symols.nupkg is an either-or to the regular that contains just the source code in addition. I changed the settings as you mentioned and added the tags to produce snupkg instead of the other. I uploaded 3.4.1-pre2 to nuget.org. Please check that one out.

@Irubataru
Copy link
Author

I also do not fully know how this works, I have been trying to figure out source link for a while now and it all seems a bit like a mess. I hope these things get more standardized because it is not good as it is now.

Anyway, I tested it out and it works great now! I am able to step into the SimSharp code with F11, and if I enable Just My Code it doesn't as expected.

Thanks a lot! <3

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.

2 participants