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

Restore as a standalone operation and remove from watch #634

Merged
merged 6 commits into from
Nov 22, 2024

Conversation

jwthomson
Copy link
Contributor

This implements @isaacabraham's suggestion in #626

@@ -45,11 +45,12 @@ Target.create "Azure" (fun _ ->
deployment |> Deploy.execute "SAFE-App" Deploy.NoParameters |> ignore)

Target.create "Run" (fun _ ->
run dotnet [ "restore"; "SAFE.App.sln" ] "."
Copy link
Member

Choose a reason for hiding this comment

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

Have you tested this when creating a new project from the template? I assume that this will break in most cases as the sln won't be called SAFE.App.sln

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My belief is that the templating will replace this but I will double check and report back!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does do the replace but for the test path I used it did the replace in such a way that the sln path's hyphens were transformed to underscores so it wouldn't work. Will dig into the templating a bit to see if I can figure out how we can avoid this.

@Larocceau Larocceau merged commit d91c3bb into master Nov 22, 2024
1 check passed
@Larocceau Larocceau deleted the dont-auto-restore branch November 22, 2024 14:25
@isaacabraham
Copy link
Member

isaacabraham commented Nov 25, 2024

@Larocceau @mattgallagher92 I've just looked at this - it is not what we discussed in the review.

  1. There is no need to restore the solution and then build shared.
  2. We simply need to build the solution. This will implicitly restore everything.
  3. The same needs to be applied when running tests - simply build the solution (which contains the test projects anyway). This will do everything required in terms of building and restoring.
  4. watch run has been left without a --no-restore here - has that been tested? This looks like it'll have exactly the same issue. https://github.com/SAFE-Stack/SAFE-template/blob/master/Content/default/Build.fs#L71

I'm going to reopen #626 as this does not appear to be a 100% fix (although I'm very happy to eat humble pie and be corrected!).

@Larocceau
Copy link
Contributor

@isaacabraham my bad, sorry about that!

1 & 2: Ok, I assumed doing it this way would be a bit faster. Just for my knowledge, what is the benefit of doing a full build here?
3 & 4: Fair enough! Did not think about it. Again, my bad, will be more diligent next time!

@isaacabraham
Copy link
Member

isaacabraham commented Nov 25, 2024

Building the Shared project is essentially going to do the same as building the solution anyway as the shared project is referenced at the very bottom of the build chain / dependency tree, and build implicit does a restore (unless, of course, you use the --no-restore argument!).

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.

4 participants