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

docs: better explain how to use pre-releases for dev/testing #373

Merged

Conversation

ShootingKing-AM
Copy link
Contributor

There is no generated static folder in case of aw-server-rust (unlike aw-server-python), even while packaging rust server, files are directly taken from aw-server-rust/aw-webui/dist/.

aw-server-rust/Makefile

There is no `static` folder in case of aw-server-rust (unlike aw-server-python), even while packaging rust server, files are directly taken from `aw-server-rust/aw-webui/dist/`.

https://github.com/ActivityWatch/aw-server-rust/blob/7c2b31f173194d75634079128a27ed06d83365b1/Makefile#L86
@ShootingKing-AM ShootingKing-AM changed the title fix: Wrong static webfiles location in Readme docs: Wrong static webfiles location in Readme Oct 10, 2022
@ErikBjare
Copy link
Member

ErikBjare commented Oct 12, 2022

Files are only taken from aw-webui/dist/ if run from source. In a normal installation, the files will indeed be under aw-server-rust/static in the installation directory (/opt/activitywatch/aw-server-rust/static exists for my installation).

Closing. If I've misunderstood, do let me know.

@ErikBjare ErikBjare closed this Oct 12, 2022
@ShootingKing-AM
Copy link
Contributor Author

ShootingKing-AM commented Oct 12, 2022

The documentation is a little confusing, i thought it was taking about the soruceTree's aw-server dir when ...or run make build from the aw-server parent directory to rebuild and copy the assets for you... - thought the aw-server directory is the sourceTree's (instead of intended installation aw-server directory. Since running make build in installation aw-server will not yield anything.

Here sourceTree means the folder where we git clone'd or the folder containing the source files.

When building from source in sourceTree, makefile of aw-server-python will create a static directory unlike makefile of aw-server-rust, so i was searching for sourceTree/aw-server-rust/static directory which doesnt exist (for 2 hrs😢)

I still feel that the doc is still confusing, my recommendation,

  • Either sync the two makefiles and create a static dir in sourceTree when building from source
  • Or, Change readme like
Either copy the assets manually, or run make build from the aw-server parent directory to rebuild and copy the assets for you.

to

Copy the assets manually from `make build` output.

Intended use: Improves dev on-boarding

@ErikBjare ErikBjare reopened this Oct 12, 2022
@codecov
Copy link

codecov bot commented Oct 12, 2022

Codecov Report

Base: 25.58% // Head: 25.58% // No change to project coverage 👍

Coverage data is based on head (6d8a4b8) compared to base (1980a40).
Patch has no changes to coverable lines.

❗ Current head 6d8a4b8 differs from pull request most recent head 0284bbe. Consider uploading reports for the commit 0284bbe to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #373   +/-   ##
=======================================
  Coverage   25.58%   25.58%           
=======================================
  Files          26       26           
  Lines        1442     1442           
  Branches      226      223    -3     
=======================================
  Hits          369      369           
- Misses       1020     1044   +24     
+ Partials       53       29   -24     
Impacted Files Coverage Δ
src/queries.ts 47.61% <0.00%> (ø)
src/util/time.ts 29.31% <0.00%> (ø)
src/util/color.ts 31.66% <0.00%> (ø)
src/stores/buckets.ts 5.79% <0.00%> (ø)
src/stores/activity.ts 32.19% <0.00%> (ø)
src/stores/settings.ts 21.95% <0.00%> (ø)
src/stores/categories.ts 63.15% <0.00%> (ø)
src/visualizations/summary.ts 0.00% <0.00%> (ø)
src/visualizations/sunburst-clock.ts 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ErikBjare
Copy link
Member

Ah, I can certainly see how that is confusing.

I think changing around how the source tree folder structure looks so that it looks the same as for the installation isn't really possible due to constraints of packaging (it might be, but it would be a mess to change). There will always be a path disparity, and the current one is good enough.

But I'm all for improving the language of the README to make it clear.

Change to: Copy the assets manually from make build output.

I think this suggestion is good.

Could you update this PR with this and maybe other changes that would clarify/simplify it? I'll take a second pass at it when you're done :)

@ShootingKing-AM
Copy link
Contributor Author

Updated with some restructuring too :)

Copy link
Member

@ErikBjare ErikBjare left a comment

Choose a reason for hiding this comment

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

Nice, just a few things.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@ErikBjare ErikBjare changed the title docs: Wrong static webfiles location in Readme docs: better explain how to use pre-releases for dev/testing Oct 13, 2022
@ErikBjare ErikBjare merged commit 3361706 into ActivityWatch:master Oct 13, 2022
@ErikBjare
Copy link
Member

Merged!

Thanks for this @ShootingKing-AM ❤️

@ShootingKing-AM
Copy link
Contributor Author

Thanks for the quick review and merge! 🎉

serve is Vue's own recommended way of http server. But both do the same thing, py httpd should also do the task. Thanks !

ref: https://cli.vuejs.org/guide/deployment.html#previewing-locally

@ShootingKing-AM ShootingKing-AM deleted the ShootingKing-AM-patch-1 branch October 13, 2022 12:42
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