Open-Source-Orchard-Core-Extensions
Open-Source-Orchard-Core-Extensions copied to clipboard
Lombiq
OSOE-819: Constant from JSON source generator
For some reason I get these with your latest commit:
But if I check out the commit before that (https://github.com/Lombiq/Open-Source-Orchard-Core-Extensions/pull/734/commits/92bbfa5eb21dd92e827d1697b7080a4c6f8df959) and do a clean and rebuild then it works. However by "works" I mean that I get no errors or warnings in VS from "Build Only". I still see IntelliSense errors:
This pull request has merge conflicts. Please resolve those before requesting a review.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
@sarahelsaig please try again with the latest version, as for the remaining warnings in visual studio I can't seem to fully get rid of them but they don't seem to be an issue as the build does actually complete succesfully and the constant values are actually showing up in the editor also.
It works in VS without any build errors or warnings (at least on a freshly cloned repository), so that's good. But I get this warning in Intellisense-only mode:
The dll file exists in that location, so this makes no sense. I would say we can ignore it, but I would also say "throw VS in a volcano" so I'm biased here. @Piedone , what's your opinion on this warning?
I really don't want us to have a warning that's actually OK. It's detrimental conditioning for the development team.
It doesn't matter if it's generated somewhere, but it shouldn't be surfaced as a warning. If you can turn it into a message, or otherwise hide it, then that's fine.
@sarahelsaig @Piedone After trying this out many more times today I found the following: Warning shows up when you do a clean pull and build solution for the first time. If you close Visual Studio right after that and restart it the warning never seems to show up again even after cleaning and rebuilding the solution.
So it does really look like a weird issue with VS in this instance. We could add a <NoWarn> for this specific warning code to each project that uses the source generator but not sure that it's really the solution and seems kind of ugly.
Let me know what you both think we should do in this case
Related, perhaps? https://github.com/dotnet/roslyn/issues/54899 Though it's about native assemblies. Isn't the issue related to the assembly targeting netstandard? It can target .NET 8 too, it'll be used by OC modules, which are all .NET 8.
Related, perhaps? dotnet/roslyn#54899 Though it's about native assemblies. Isn't the issue related to the assembly targeting netstandard? It can target .NET 8 too, it'll be used by OC modules, which are all .NET 8.
I did come across that post and it does seem to be about referencing native assemblies from the source generator. In our case it's about a local reference from a project referencing / using the source generator.
As for targetting netstandard2.0 from the source generator this is unfortunately a hard requirement for source generators to work.
Then please suppress this warning if it's possible to do just the affected projects, commented why it's necessary, pointing to some common docs in HL where you also tell consumers to do this in their projects.
@sarahelsaig @Piedone Even when adding the NoWarn to try and suppress this warning I am still getting it in VS the first time I clone this project. Can either of you verify that it's still the case?
If so I am kind of running out of ideas on how to suppress this warning any further or what else to try, and it's starting to take up way too much time digging into this niche issue. Open to any suggestions though so please let me know how you think we should proceed.
For me too. However, the DLL actually indeed doesn't exist for me when opening the solution. So, perhaps you need to fix that instead?
For me too. However, the DLL actually indeed doesn't exist for me when opening the solution. So, perhaps you need to fix that instead?
Indeed it doesn't exist when the project is freshly cloned and opened for the first time in VS. It does however exist after the first build, I guess that is just a quirk of referencing source generators locally in this way. After that first build the warning doesn't seem to come back anymore (even after a clean of the solution).
Again I am open for suggestions on how to either solve this or suppress the warning further but as I mentioned I have kind of run out of things to try at this point.
Only thing I can think of right now is maybe open up an issue over at https://github.com/dotnet/roslyn and see if they have any ideas?
Well, I suggest fixing that the DLL doesn't exist :). The warning is not a false positive. You can either make the DLL exist (perhaps by hooking into an MSBuild target that's executed when the project is loaded and doing some kind of pre-build) or the reference to only be active when it exists (you can add conditions to csproj elements).
Feel free to additionally ask under the Roslyn repo too (though I suggest a discussion, since this is a question, not a bug of Roslyn).
This pull request has merge conflicts. Please resolve those before requesting a review.
Well, I suggest fixing that the DLL doesn't exist :). The warning is not a false positive. You can either make the DLL exist (perhaps by hooking into an MSBuild target that's executed when the project is loaded and doing some kind of pre-build) or the reference to only be active when it exists (you can add conditions to csproj elements).
Feel free to additionally ask under the Roslyn repo too (though I suggest a discussion, since this is a question, not a bug of Roslyn).
@Piedone See the reply on the discusson https://github.com/dotnet/roslyn/discussions/72937#discussioncomment-9115359 please let me know how you want to proceed.
Please provide me with options.
The only option I see is at the moment is: Accept the fact that there will be a warning on a fresh clone of the repo until a build is done (and in some cases VS is restarted - because it's VS) - as this seems to just be an issue that's been accepted as a quirk of local source generators as per the comment in the linked discussion.
If that really is out of the question as I mentioned I'd be open to suggestions. If it's trying your earlier suggestion about a prebuild I would probably need some more help/pointers or examples on how to do that, and even then it's very likely it wouldn't even do anything in this case because source generators run differently than normal projects.
The warnings don't affect the F5 experience, so that's good. However, what I now found that if you launch the web app with (Ctrl+) F5, the warning won't go away even after a VS restart. You need to run a build with F6 instead; that fixes it after a restart.
Why is this? Is it perhaps because the DLLs in the warning still aren't build after just F5, because their project is not ultimately in the dependency chain of the web project that's being launched?
Also, please examine this:
You can either make (...) the reference to only be active when it exists (you can add conditions to csproj elements).
Also, please examine this:
You can either make (...) the reference to only be active when it exists (you can add conditions to csproj elements).
@Piedone I worked on this a bit but this kind of turns into a chicken and the egg problem. The projects that use the ConstantFromJson will not build without errors until the Source Generator project is built at least once. So this effectively turns the warning into an error at this point.
https://github.com/Lombiq/Helpful-Libraries/compare/cafd6771b10d311e775c244ac0032563d6d42bbb...6eb71ac29c29e83ee51d2264f03ba4f0bcf98420#diff-bc5ef2f21ac09705d622b943b9261ec0249e73a12bb7d18b7aa435cc6027e45fR28
Unless we can make sure that the source generator project is always built before any other projects are built both in the pipeline and inside the IDE's I don't think this really solves anything.
Having project dependencies should ensure that. So, is it perhaps because the DLLs in the warning still aren't built after just F5, because their project is not ultimately in the dependency chain of the web project that's being launched?
@Piedone Adding a reference to source generator directly from the web project like this do you mean? Or am I misunderstanding? https://github.com/Lombiq/Open-Source-Orchard-Core-Extensions/pull/734/commits/0703e31484046bd251015d477d31d638272bfb1c#diff-c9834d7af92e9192183f3142238d4454fab88d76565fd17f60d966c4d1917bf5R26
Seems like for VS at least it works and the constant in the test project is actually generated. But in the pipeline the build seems to fail
Not necessarily from the web project, but if that's required, then that's OK but needs to be documented. Rather, when you start from the web project's dependencies, all those projects should be in the dependency chain eventually (can be just the 5th level, but still) that need to be built for this to work.
F6 builds the whole solution, i.e. all projects, while F5 only builds the web project and its dependencies (i.e. only what's needed for the web project). That suggests to me that the project whose DLL VS warns about missing is not in the web project's dependency chain.
F6 builds the whole solution, i.e. all projects, while F5 only builds the web project and its dependencies (i.e. only what's needed for the web project). That suggests to me that the project whose DLL VS warns about missing is not in the web project's dependency chain.
Right currently the only project that references it is the test project, I added it to the web project directly just as a test and as I mentioned it does seem to do the build fine in VS for me at least on a fresh clone but in the pipeline build doesn't get through
That's behind the whole issue, then. How do the consumer projects work then in the first place? They need to depend on the sourcegen project somehow, because that'll also be needed for NuGet publishing (https://lombiq.atlassian.net/wiki/spaces/DEV/pages/786857987/Managing+releases+of+open-source+projects) when we build each project in their repository alone.
So, please also do an alpha publish of these projects (see docs) to make sure that works. That'll ensure that the build also works.
This pull request has merge conflicts. Please resolve those before requesting a review.
This pull request has merge conflicts. Please resolve those before requesting a review.
So, please also do an alpha publish of these projects (see docs) to make sure that works. That'll ensure that the build also works.
I have read through this but I am honestly really sure how that applied in this instance. I did however play around more with the MSBuild process and I think I did manage to get rid of the warning setting it up this way. Let me know what you think
Each submodule should be possible to build on its own, out of the context of the solution, with only its package dependencies. This is necessary for NuGet publishing. We need this to work, so please check with alpha NuGet publishes.
The VueJs and Base Theme submodules are on their devs here, not issue/OSOE-819, and thus aren't using the source generator. Updating them to that makes the build fail:
Before anything further, please make sure that the submodules are all up-to-date, and the issue is fixed after you recursively clean the repo (https://lombiq.atlassian.net/wiki/spaces/DEV/pages/56492060/Git+tips#:~:text=Recursive%20repository%20purge) and open VS after that.
@Piedone I think something is going wrong here because in the current version of this I haven't touched basetheme and vuejs modules yet.
After the first time we reverted everything I started over on this issue and created a new branch, it looks like now you still have some of these changes from before that happened? The only project that currently has been updated to use the source generator is the HelpfulLibraries Test project. I wanted to make sure that this all is fine before re-implementing it in the VueJS and BaseTheme
I see, OK then. I was confused by PRs being open for the other projects too. Yes, now looks good, thank you.
Going forward, you two don't need my help to test this, since anyone can install Visual Studio. Please make sure that it builds properly in VS, on the first try, also with just hitting F5. Also, make sure that the projects can be published on their own as NuGet. The technological decisions to get there is up to you.
I'm unsubscribing here, please only @mention me if it's really necessary.