sugarlabs / aslo-v3

Upcoming Software Center for SugarLabs
2 stars 3 forks source link

Add local resources to reduce DNS requests and fix Travis Build (GCI) #10

Closed hkatzdev closed 6 years ago

hkatzdev commented 6 years ago

Hello! This pull request mainly edits the layout template and googleFonts.css and adds many new files. I found these files using grep -RliE "fonts.gstatic.com|maxcdn.bootstrapcdn.com|cdnjs.cloudflare.com|ajax.googleapis.com" ./, which returned

//env/lib/python3.7/site-packages/flask_bootstrap/__init__.py
.//env/lib/python3.7/site-packages/flask_bootstrap/__pycache__/__init__.cpython-37.pyc
.//aslo/web/static/css/googleFonts.css
.//aslo/web/templates/partials/layout.html

For the layout template, I tried to find the javascript libraries online and download the folder in order to avoid some errors. When I originally downloaded the javascript files and put them in the javascript folder, it would give errors due to checking for other files, like CSS or fonts, using ./. I could not find a download for 4.0.2 of bootstrap-material-design, but using version 4.1.1 did not cause any errors. Unfortunately, I could not figure out how to use url_for in the CSS file, but using ../fonts/[insert name] worked on my end. I used url_for for the javascript libraries and favicon in the layout file. After doing this, grep -RliE "fonts.gstatic.com|maxcdn.bootstrapcdn.com|cdnjs.cloudflare.com|ajax.googleapis.com" ./, simply returns .//web/static/fonts/fonturls.txt, which is where the font urls are kept to easily add or subtract fonts. I have included screenshots of the resource tab before and after my commits.

Requests without commits: beforecommits

Requests with commits: localresources

I also tried to include a fix for the Travis builds. It seems that flake8 did not like escape sequences in the regex for the match variable, so I just used an equivalent and flake8 no longer gives any errors on my computer. However, I do not know what this does or how to test it, although since they are equivalent regex it should work the same way. Update: It has passed the Travis build test. Update 2: The error seems to be related to regex escapes not being valid python escapes (source). It seems that it is recommended to add the r prefix and make it a raw string instead of replacing the escapes. Neither give errors on my computer, but I do not know how to check the functionality of this.

Thank you for your tips on downloading other apps to assist me using git - I made a mistake in a commit once, and it was a lot easier typing git undo than it was trying to figure out how to revert it and how that affects history.

For reference, this is for Google Code-In 2018. My mentors are @walterbender @i5o and @vipulgupta2048. Thank you @quozl for looking over my last pull request and giving me some tips on how to improve.

vipulgupta2048 commented 6 years ago

@quozl The size of the repository is bound to increase by a lot from this since more data would now be local, but the DNS queries have been greatly reduced by the good work of @HarrisonLKatz, Can we work with this trade-off? @jatindhankhar @i5o Can you review as well. PS. Really good work on the Travis build.

jatindhankhar commented 6 years ago

Hi @HarrisonLKatz , Thank you for the contribution. Good work fixing the flake issues :+1:

My concern is same as @vipulgupta2048 's, although it decreases the requests made to external resources, it increases overall repo size, most of which is not code. Adding Material Bootstrap, Font awesome and other minified files are okay but adding all webfonts (ending with ..woff2) seems excessive to me. Ideally these assets should be handled by a frontend pre-processor like Bower. See following links (https://realpython.com/the-ultimate-flask-front-end/) , (https://stackoverflow.com/a/45968868)

It would help us in self hosting resources, without committing them to repo. I am in favour of the bower approach, @quozl, what do you think about the same?

quozl commented 6 years ago

I'm in favour of merging this pull request as is.

Size downloaded by a browser has not increased. Good.

Yes, repository size has increased, one order of magnitude, from 476 KB to 6.6 MB. However, the advantages are;

Yes, you might use tools to identify what resources are actually used, and delete files that are not used. An easy method is to run your coverage test, generate from server logs a list of files used, and then compare against a list of all files. For the size of this repository (287 files as of 536df2a), this may be easiest.

A disadvantage you both missed is that sugarlabs.org infrastructure must send more data for each user. However, the data is sent and cached, so it is not as severe an impact as it could be.

There are other reasons why I'm not planning to switch from v1 to v3 for the millions of laptops we've deployed any time soon; several missing features prevent switching. Discussed on sugar-devel@ some months ago. So the demand for v3 will come from users who install Sugar Live Build or SoaS. That demand can be measured by counting user agent statistics from v1 logs.

quozl commented 6 years ago

@vipulgupta2048, why do you want the repository to be a small size? I'm used to sub-gigabyte Linux kernel repositories. git scales well.

hkatzdev commented 6 years ago

Hello! If this helps in any decision, I am perfectly fine with redoing the fonts. In fact, I would be excited to learn how to use Yarn and Webpack, since Bower is discontinued. I found a node plugin here https://github.com/makovich/google-fonts-offline and a Webpack plugin here https://github.com/gabiseabra/google-fonts-webpack-plugin. However, I would probably need someone to help me use them - I have node, Webpack, and yarn installed on my computer and have set them up using init, but I do not know how to use the plugins to download the fonts locally. Also, I would probably need more than a day, though not by much - 2 days should be fine. Thank you for reviewing my pull request. Thank you, Harrison

vipulgupta2048 commented 6 years ago

@quozl, that is true. I have less experience in the matter of working with repositories of larger sizes. As I have heard that pulling them down takes up loads of time. Regardless, if the pull request looks good, then can we move forward to merging it. Or we can work on his suggestion (I can extend the deadline if you feel so) The student has been waiting long enough. I will leave the decision to you.

quozl commented 6 years ago

@vipulgupta2048, thanks. Please learn how to use --depth option for git clone, to create a shallow clone with truncated history. Regarding merging the pull request, I'm fine with doing so (as before), you've said okay, but I haven't heard from @jatindhankhar about it.

@jatindhankhar, if you approve, please merge, thanks.

hkatzdev commented 6 years ago

Hello, I don't know what happened, but I tried running this again and flake is giving tons of errors. I tried downloading it again from GitHub so it is probably something with my computer, but it would probably be a good idea if I investigate it before this is merged. It is so strange - it only started happening this morning, but it won't go away. Thank you, Harrison EDIT: 158,560 lines of warnings to be exact - this might take me a little while to go through :). If anyone has any ideas, I attached it below. log.txt

quozl commented 6 years ago

All those messages are related to files starting with directory env which isn't in git, so it's not a problem for us, it's a problem for you. You might get rid of env if you don't need it.

hkatzdev commented 6 years ago

All those messages are related to files starting with directory env which isn't in git, so it's not a problem for us, it's a problem for you. You might get rid of env if you don't need it.

I think I figured it out - it installed some of the packages in my local folder for some reason, probably when I was trying to figure out something. I am deleting them from my local folder and I'll try again - glad it is not something serious.

quozl commented 6 years ago

No worries. Have a look at the documentation for git clean, it's very handy, ... and good for losing things.

hkatzdev commented 6 years ago

After running it again, it now gives me errors outside of .env, such as

./aslo/api/release.py:182:7: W605 invalid escape sequence '\w'
./aslo/api/release.py:182:12: W605 invalid escape sequence '\d'
./aslo/api/release.py:182:17: W605 invalid escape sequence '\d'

I fixed these errors above, so I am quite a bit confused - I feel like I'm not cloning the right repository... Edit as I am thinking this - switching into the right branch may help - I'll try again in the right branch.

Edit2: CDing into the aslo directory could help too :/. Thats probably where my env errors come from.

jatindhankhar commented 6 years ago

@HarrisonLKatz, This was the initial cause of linter issue which broke our build. It seems to be started few weeks with then new release of flake (https://github.com/biopython/biopython/issues/1833) ? Currently there are no version lock for packages (https://github.com/sugarlabs/aslo-v3/blob/master/requirements.txt#L1) , we can lock down flake8 to a specific version @quozl
PR is fine. If few more days can be devoted to following task, we can try webpack approach. Otherwise it is fine, it can be picked up some time in future.

quozl commented 6 years ago

Thanks. Consensus is unanimous, so merging.

@HarrisonLKatz, a wild guess, perhaps you're yet to reliably notice what directory you are in, how things you do have side-effects on the directory, and the state of the git repository as a whole. Key to these learnings is to make mistakes as fast as you can, recover from them, and be able to learn from them. A visualisation of data that git touches may be helpful. Click on the different columns.

quozl commented 6 years ago

@HarrisonLKatz, our web site at http://sugarlabs.org/ repository www-sugarlabs would also benefit from the same kind of fix. It takes half a minute to load because of it.

hkatzdev commented 6 years ago

@HarrisonLKatz, our web site at http://sugarlabs.org/ repository www-sugarlabs would also benefit from the same kind of fix. It takes half a minute to load because of it.

Should I manually download the files like I did here, or should I try to use webpack for www-sugarlabs?

quozl commented 6 years ago

Don't know, sorry. Best to discuss over there, with a wider group of interest. See https://github.com/sugarlabs/www-sugarlabs/issues number 218, intentionally not linked because I've no idea how relevant this pull request is.