Added vendor dir for js/css libs; Documented sources (#1484) #2241

MTecknology · 2017-07-31T21:24:11Z

This PR resolves #1484 and moves vendor'ed javascript (and other) libraries to public/vendor/. It also moves the documentation to vendor/librejs.html and includes documentation for both js and non-js libs.

With this commit, it is easy to prove only free/open-source libraries are being used by gitea, which helps meet GNU ethical repository criteria (#1524).

This commit mostly addresses #1484 by moving vendor'ed plugins into a vendor/ directory and documenting their upstream source and license in vendor/librejs.html. This also proves gitea is using only open source js/css libraries which helps toward reaching #1524.

The version of this file in use is located at: vendor/plugins/highlight/github.css

sapk · 2017-08-01T19:12:39Z

LGTM
Just, why do you choose to put all under /plugins ? Why not keep some under sub-folder /libs like jquery, vue or semantic ?

A SafeJS function was added to templates/helper.go to allow keeping comments inside of javascript. A javascript comment was added in the header of templates/base/head.tmpl to mark all non-inline source as free. The librejs.html file was updated to meet the current librejs spec. I have now verified that the librejs plugin detects most of the scripts included in gitea and suspect the non-free detections are the result of a bug in the plugin. I believe this commit is enough to meet the C0.0 requirement of #1534.

MTecknology · 2017-08-02T05:32:17Z

@sapk

There were times where the CSS components weren't kept with the JS bits, but that wasn't consistent. I moved each lib into it's own directory partly to add consistency; I also wanted to be able to include a license file for each vendor'ed library to clearly identify each project as a separate work and clearly show the license terms of that source (which also helps meet license terms).

I kept them under plugins/ instead of breaking them into the other sub-directories because I wasn't able to discern the logic to the current layout.

Example:

jquery-1.11.3 was under public/js/
jquery.are-you-sure was under public/js/libs/
two other jquery libs were under public/plugins/<lib-name>/

public/js/libs/
    clipboard-1.5.9
    emojify-1.1.0
public/plugins/
    highlight-9.11.0
    simplemde-1.10.1

Even in the source, I couldn't find a reason why they should be considered different. Sorry, I didn't mean to get long-winded or ranty... just wanted to thoroughly explain my logic. :)

sapk · 2017-08-02T09:42:37Z

@MTecknology ok not a problem just to know if there were any reason. But effectively there wasn't before also ^^. Maybe we should think of using versionning for web ressources like npm or bower in the future. (not sure everyone will aggree with me ^^). still LGTM

lafriks · 2017-08-02T09:49:16Z

@sapk in the future we should move to webpack to have single js file

MTecknology · 2017-08-05T03:58:46Z

@lafriks I agree! It'd be nice to have a nice json file (or two) with a pretty script to update all libs and run unit testing. If we had that, librejs.html could be turned into a dynamic file that's built by reading existing json files. (sounds great for a lot of reasons)

For now, though, it'd be super if this could make it into 1.2.0. It'd make my life a whole super duper lot easier. :)

(P.S. I actually don't care that much about the last commit. I hope to never touch that librejs plugin again... sticking w/ noscript. I just happened to be "in the neighborhood.")

bkcsoft

A part from the question. There's no file listing which versions we have. This have to exist for this to be merged. I don't care if it's a real package.json/yarn.lock or just a plain-text file listing them, but it absolutely have to exist.

bkcsoft · 2017-08-08T09:09:04Z

public/assets/librejs/librejs.html

@@ -1,84 +0,0 @@
-<!DOCTYPE html>


Why has this file changed?

It has moved under public/vendor/librejs.html

As requested => the VERSIONS file

bkcsoft · 2017-08-08T09:14:27Z

don't care if it's a real package.json/yarn.lock or just a plain-text file listing them

With this I mean that for this PR I don't care, any updates in the future should use yarn though ;)

sapk · 2017-08-08T09:24:47Z

@bkcsoft we could do that in an other PR.

sapk · 2017-08-08T09:25:37Z

And in fact all the version are listed in archive link of librejs.html

bkcsoft · 2017-08-08T09:30:23Z

@sapk Why I hate PRs that touch 1.3k files 😂

sapk · 2017-08-08T09:51:38Z

Me too but in this case it is mostly rename.

bkcsoft · 2017-08-08T09:55:36Z

@sapk right, my point was that we need a file public/vendor/VERSIONS that just lists all versions. Since I had no idea that librejs.html had that, and therefore no one else knows that either 😉

MTecknology · 2017-08-08T20:20:40Z

@bkcsoft I think I already responded to most of those concerns. I agree, librejs.html should be dynamically generated from the output of a tool that can keep JS libs up to date. (hopefully choosing a tool that can output a "versions" file)

I also think that's something much better suited for a different PR. I tried to keep this PR to just file moves and documentation--it's big, but I tried for minimal. librejs.html wasn't listed as a move because the file contents changed almost entirely.

Also, in the future, it'd be nice to apply a template to the rendered page so it's not just a boring table but a themed page with <table>[...]</table> as the page body.

strk · 2017-08-16T06:47:40Z

LGTM

strk · 2017-08-18T08:23:40Z

This is ready to merge, why waiting for 1.3.0 ? I think it is worth having this ib 1.2.0

lunny · 2017-08-18T09:03:03Z

It's not tested well. I don't think move it to v1.2 is a good idea.

strk · 2017-08-18T16:12:14Z

@lunny 1.2.0 could be the first version with a proper Debian package, but I've understood a Debian package depends on this PR, for licensing issues.

#2285 was more dangerous to put in 1.2.0, if you ask me, as I suspect it's what broke the UI (#2331)

MTecknology · 2017-08-18T19:03:38Z

@lunny This is a large PR, but I would encourage you to review what is being changed.

Moved 3rd party libs to designated directory
Updated path references to match
Created/updated library documentation
3a. Added missing LICENSE files
Fixed issues for librejs users

This PR addresses some licensing concerns that, as @strk mentioned, are blockers for other bugs--one of those being a proper Debian package. I intentionally kept this changeset as small as possible to prevent this problem. I don't know what additional testing can be performed; if you tell me, I'll do my best to make it happen.

@bkcsoft You still have "changes requested," but I do not see what changes you are requesting. Could you please tell me so that I can make them?

bkcsoft · 2017-08-19T15:37:46Z

@MTecknology A file listing packages and versions. (no, librejs.html does not count 🙂 )

MTecknology · 2017-08-19T17:52:19Z

@bkcsoft Your comment was about the file being removed. The file was not removed, it was relocated and modified. I already commented on the inclusion of an alternate option. It's a cool idea. This PR is already big enough, isn't it? It's a really big PR and, as I mentioned, I tried hard to keep it minimal. I tried to keep out functional changes and keep it to documentation and cleanup. I disagree very much that this should be held up to wait for an entirely new and undiscussed feature.

bkcsoft · 2017-08-21T11:53:35Z

@MTecknology It's not a new feature, the version were tracked by the folder-names (jquery-1.1.3 etc), but now that information is removed and needs to be added back in some form. And as I've said, it does not have to be yarn/npm, just a straight up flat file listing deps and versions...

MTecknology · 2017-08-21T15:54:39Z

@bkcsoft That information /is/ in librejs.html... Why does that file "not count"?

bits from that file:

 +          <td><a href="/codedance/jquery.AreYouSure/archive/1.9.0.tar.gz">jquery.areyousure-1.9.0.tar.gz</a></td> 
 +          <td><a href="https://code.jquery.com/jquery-1.11.3.min.js">jquery-1.11.3.min.js</a></td> 
 +          <td><a href="/Semantic-Org/Semantic-UI/archive/2.2.1.tar.gz">semantic-UI-2.2.1.tar.gz</a></td>
 +          <td><a href="/zenorocha/clipboard.js/archive/v1.5.9.tar.gz">clipboard-1.5.9.tar.gz</a></td>

Are you looking for an additional file? Would you prefer I create librejs.yml to sit beside librejs.html that include the same information and adds an extra field just for the version?

strk · 2017-08-21T22:29:34Z

Is a machine readable format you are after, @bkcsoft ?

lunny · 2017-08-22T01:24:19Z

I found some links broken in librejs.html master version. If this PR will also fix that?

MTecknology · 2017-08-22T04:35:31Z

@lunny Yes...

The first commit in this PR:

This commit [moves] vendor'ed plugins into a vendor/ directory and document[s] their upstream source and license in vendor/librejs.html.

I'll say this again... very little actually changes in this PR. It's documentation and licensing crap. Please, look at what files changed and what file changes took place. I tried very hard to make this PR very easy to review for these exact reasons... I thought I'd succeeded... :(

bkcsoft · 2017-08-23T14:56:40Z

@MTecknology My main quarel with having them listed in librejs.html is because that file is not where I'd expect to find that information. Since you've now added a VERSION file this LGTM 🙂

lofidevops · 2017-08-23T15:10:18Z

Awesome!

lunny · 2017-08-23T15:11:08Z

And If you want to this in v1.2, please send a back port PR to branch release/v1.2

gsantner · 2017-08-23T18:46:01Z

I'm on current HEAD and there are things broken. I think it's related to this one

daviian · 2017-08-23T22:08:06Z

Furthermore integration tests are failing because of moved librejs file.

strk · 2017-08-23T22:27:11Z

Is drone not running integration tests ?

daviian · 2017-08-23T22:27:58Z

@strk @tboerger mentioned that integration tests currently run only on master branch...

This now break until upstream accepts PR #2241 [1] and a new tarball is imported with the changes. [1] go-gitea/gitea#2241

MTecknology added 2 commits July 31, 2017 15:17

Removing unused css file.

0a4d60f

The version of this file in use is located at: vendor/plugins/highlight/github.css

bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 31, 2017

bkcsoft added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 1, 2017

bkcsoft suggested changes Aug 8, 2017

View reviewed changes

lunny added this to the 1.3.0 milestone Aug 16, 2017

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 16, 2017

MTecknology and others added 2 commits August 18, 2017 11:46

Merge branch 'master' into master

fd101eb

Updating SafeJS function per lint suggestion

d711a83

MTecknology mentioned this pull request Aug 21, 2017

Support Reverse Proxy Email #2347

Closed

7 tasks

Added VERSIONS file, per request

926cb40

bkcsoft approved these changes Aug 23, 2017

View reviewed changes

bkcsoft merged commit a915a09 into go-gitea:master Aug 23, 2017

This was referenced Aug 23, 2017

Icon Fonts missing since moving vendor dir files #2372

Closed

Build failes since moving vendor dir files #2373

Closed

MTecknology mentioned this pull request Aug 23, 2017

Move themes to plugin directory. Fixes #2372 #2375

Merged

MTecknology added a commit to go-gitea/debian-packaging that referenced this pull request Aug 1, 2018

Added 5 js libs to gitea-common; 15 to go.

ac68d3d

This now break until upstream accepts PR #2241 [1] and a new tarball is imported with the changes. [1] go-gitea/gitea#2241

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Added vendor dir for js/css libs; Documented sources (#1484) #2241

Added vendor dir for js/css libs; Documented sources (#1484) #2241

Conversation

MTecknology commented Jul 31, 2017

sapk commented Aug 1, 2017

MTecknology commented Aug 2, 2017 • edited Loading

sapk commented Aug 2, 2017 • edited Loading

lafriks commented Aug 2, 2017

MTecknology commented Aug 5, 2017 • edited Loading

bkcsoft left a comment

Choose a reason for hiding this comment

bkcsoft Aug 8, 2017

Choose a reason for hiding this comment

sapk Aug 8, 2017

Choose a reason for hiding this comment

MTecknology Aug 23, 2017

Choose a reason for hiding this comment

bkcsoft commented Aug 8, 2017

sapk commented Aug 8, 2017

sapk commented Aug 8, 2017

bkcsoft commented Aug 8, 2017

sapk commented Aug 8, 2017

bkcsoft commented Aug 8, 2017

MTecknology commented Aug 8, 2017 • edited Loading

strk commented Aug 16, 2017

strk commented Aug 18, 2017

lunny commented Aug 18, 2017

strk commented Aug 18, 2017

MTecknology commented Aug 18, 2017

bkcsoft commented Aug 19, 2017

MTecknology commented Aug 19, 2017 via email • edited Loading

bkcsoft commented Aug 21, 2017

MTecknology commented Aug 21, 2017

strk commented Aug 21, 2017

lunny commented Aug 22, 2017

MTecknology commented Aug 22, 2017

bkcsoft commented Aug 23, 2017

lofidevops commented Aug 23, 2017

lunny commented Aug 23, 2017

gsantner commented Aug 23, 2017

daviian commented Aug 23, 2017

strk commented Aug 23, 2017 via email

daviian commented Aug 23, 2017 • edited Loading

MTecknology commented Aug 2, 2017 •

edited

Loading

sapk commented Aug 2, 2017 •

edited

Loading

MTecknology commented Aug 5, 2017 •

edited

Loading

MTecknology commented Aug 8, 2017 •

edited

Loading

MTecknology commented Aug 19, 2017 via email •

edited

Loading

daviian commented Aug 23, 2017 •

edited

Loading