-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Build all of the vector dir in the build process #2558
Conversation
So the whole vector/ directory can now be removed during the build process and we don't ship random files that end up in the vector directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally seems like a great idea.
It will need to carry some sort of health warning to people who have locally-modified config.jsons which need moving. In fact I'm somewhat tempted to suggest leaving it where it is and making a new build directory. WDYT?
"build:dev": "npm run build:emojione && npm run build:css && npm run build:bundle:dev", | ||
"dist": "scripts/package.sh", | ||
"start:res": "parallelshell \"cpx -w \\\"{src/skins/vector/fonts,src/skins/vector/img}/**\\\" vector/\" \"cpx \\\"{res/media,res/vector-icons}/**\\\" vector/\"", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the idea of nesting parallelshells makes me sad. Is there a reason these need to be separate cpx
invocations?
Failing that the second cpx needs a -w.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if they're the same one, cpx copies it as a whole tree to vector/res[...] and vector/src[...] :(
Yeah, possibly. OTOH, renaming the dir will break our deploy script, although maybe this is more fixable. |
People may have config files in vector/ so it would be nonideal if we started overwriting them / blowing them away.
Rewritten to use |
/vector/config.json | ||
/vector/index.html | ||
/vector/olm.* | ||
/vector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/vector/webapp/
@@ -27,23 +27,27 @@ | |||
"matrix-react-parent": "matrix-react-sdk", | |||
"scripts": { | |||
"reskindex": "reskindex -h src/header", | |||
"build:emojione": "cpx \"node_modules/emojione/assets/svg/*\" vector/emojione/svg/", | |||
"build:res": "cpx \"{src/skins/vector/fonts,src/skins/vector/img}/**\" webapp/ && cpx \"{res/media,res/vector-icons}/**\" webapp/", | |||
"build:config": "cpx config.json webapp/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so wasn't the point to leave config.json
in vector
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to keep it in the root directory so it's more obvious to find: I don't really want a vector
directory hanging around for the sole purpose of containing the config: people will have to move the config file but I don't think that's a big deal.
LGTM modulo update to jenkins.sh |
and make them re-use the package script rather than doing their own thing
While we're breaking things anyway
Fixed - quick sanity check? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks plausible otherwise
|
||
# gzip up ./vector | ||
rm vector-*.tar.gz || true # rm previous artifacts without failing if it doesn't exist | ||
rm packages/vector-*.tar.gz || true # rm previous artifacts without failing if it doesn't exist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
package.sh
seems to build to dist/, not packages/ ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yes, because I changed that the other day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, but will leave you to merge and frob jenkins
So the whole vector/ directory can now be removed during the build
process and we don't ship random files that end up in the vector
directory.