Skip to content
This repository was archived by the owner on Jun 10, 2024. It is now read-only.

build: introduce a workspace#218

Closed
rvolosatovs wants to merge 3 commits intowasmCloud:mainfrom
rvolosatovs:build/workspace
Closed

build: introduce a workspace#218
rvolosatovs wants to merge 3 commits intowasmCloud:mainfrom
rvolosatovs:build/workspace

Conversation

@rvolosatovs
Copy link
Member

@rvolosatovs rvolosatovs commented Mar 1, 2023

Feature or Problem

Introduce a workspace to simplify maintenance, speed up builds by
facilitating artifact reuse and minimize dependency graph (and make it
consistent)

Note, a custom resolver introduced in 6c68235 does not seem to be
necessary, since the crate builds just fine without it and is inconsistent with other providers.

Removed to make things simpler with the workspace

Related Issues

wasmCloud/wasmcloud-test#22

Release Information

Consumer Impact

Testing

Built on platform(s)

  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
  • x86_64-windows

Tested on platform(s)

  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
  • x86_64-windows

Unit Test(s)

Acceptance or Integration

Manual Verification

Built locally and tested in CI

A custom resolver was introduced in 6c68235, which does not seem to be
necessary, since the crate builds just fine without it and is
inconsistent with other providers

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs rvolosatovs marked this pull request as ready for review March 1, 2023 13:20
Introduce a workspace to simplify maintenance, speed up builds by
faciliating artifact reuse and minimize dependency graph (and make it
consistent)

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Add symlinks to workspace `target` until
wasmCloud/wasmcloud-test#22
is merged and integrated into this repository removing the need for
on-disk test configuration

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
This was referenced Mar 1, 2023
@autodidaddict
Copy link
Member

My only concern is the symbolic links in the target folders. I would rather fix the makefiles and other things than use symlinks and potentially cause problems with portability/windows.

@rvolosatovs
Copy link
Member Author

My only concern is the symbolic links in the target folders. I would rather fix the makefiles and other things than use symlinks and potentially cause problems with portability/windows.

I agree with the concern, however note that the symlinks are removed in #219. If wasmCloud/wasmcloud-test#22 were to be merged and a new version of the crate was released, I could cherry-pick that commit here and there'd be no need for symlinks in main

@autodidaddict
Copy link
Member

okay. I was just looking at this PR in isolation so I didn't know that the symlinks were being removed.

@rvolosatovs
Copy link
Member Author

okay. I was just looking at this PR in isolation so I didn't know that the symlinks were being removed.

see also commit message 859c146

@autodidaddict
Copy link
Member

okay. I was just looking at this PR in isolation so I didn't know that the symlinks were being removed.

see also commit message 859c146

Thanks for pointing that out. When I use the review button in Github, I don't see commit mesages.

Copy link
Member

@brooksmtownsend brooksmtownsend left a comment

Choose a reason for hiding this comment

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

Looks great, I was concerned this would force us to compile the entire repo for a single provider but that doesn't seem to be the case.

@rvolosatovs Just to be sure, have you attempted to run make par-full in these new directories? I think you will need to add the workspace as a volume in order for the build to resolve the dependency versions. This is a check we run on-release so it might not be caught with our build CI

@brooksmtownsend
Copy link
Member

@rvolosatovs if you wouldn't mind rebasing and checking out my last comment, I think we can merge this in. Just need to get that addressed

@rvolosatovs
Copy link
Member Author

@rvolosatovs if you wouldn't mind rebasing and checking out my last comment, I think we can merge this in. Just need to get that addressed

Since we're moving these to wasmcloud/wasmcloud, I prefer not to spend more time on this, especially since I'm not familiar with the build process here. Feel free to address any issues

@rvolosatovs rvolosatovs deleted the build/workspace branch June 5, 2023 14:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants