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

Replace rakyll/static with native embedding #706

Merged
merged 14 commits into from
Mar 17, 2021

Conversation

Darkren
Copy link
Contributor

@Darkren Darkren commented Mar 1, 2021

Did you run make format && make check?
Yes
Fixes #703

Changes:

  • rakyll/static was replaced with native embedding;
  • make build-ui was modified to make use of native embedding.

cmd/skywire-visor/commands/root.go Show resolved Hide resolved
Copy link
Member

@jdknives jdknives left a comment

Choose a reason for hiding this comment

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

@Darkren good job overall. We need to bump Travis to Go 1.16, update the README and please answer my query above. Then this can go. Well done!

Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@Senyoret1 Senyoret1 left a comment

Choose a reason for hiding this comment

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

I’m not sure about how the embedding procedure works, but it worked well in my test, so appart from my comments I don’t find any problems 👍

Makefile Outdated Show resolved Hide resolved
Makefile Outdated

build-static: host-apps-static bin-static ## Build apps and binaries. `go build` with ${OPTS}
build: move-built-frontend host-apps bin ## Install dependencies, build apps and binaries. `go build` with ${OPTS}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think having move-built-frontend here is a problem. With this, now the dist folder will have to be present every time anybody wants to build the binaries. As it was removed from the repo, that would mean the need to have all the front-end tools intalled and build the UI each time befor building the binaries, which would be inconvinient. Also, the process would be complicate even having the front-end tools, because make build-ui calls make move-built-frontend, which would remove the dist folder just after finishing, so a call to make build just after calling make build-ui would fail. I think the solution would be to remove move-built-frontend from here.

Maybe the same applies to build-static.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Senyoret1 agreed. but we definitely need dist in cmd/skywire-visor to build binaries now. native embedding requires that files being embedded are ready at compile time. I removed this command but left it in build-ui. so that built ui in the end lies where needed

@jdknives jdknives merged commit 78454be into skycoin:develop Mar 17, 2021
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.

3 participants