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

Build all of the vector dir in the build process #2558

Merged
merged 9 commits into from
Nov 11, 2016
Merged

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Nov 8, 2016

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.

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.
Copy link
Member

@richvdh richvdh left a 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/\"",
Copy link
Member

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.

Copy link
Member Author

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[...] :(

@richvdh richvdh assigned dbkr and unassigned richvdh Nov 8, 2016
@dbkr
Copy link
Member Author

dbkr commented Nov 8, 2016

Yeah, possibly. OTOH, renaming the dir will break our deploy script, although maybe this is more fixable.

dbkr added 3 commits November 8, 2016 15:30
People may have config files in vector/ so it would be nonideal
if we started overwriting them / blowing them away.
@dbkr
Copy link
Member Author

dbkr commented Nov 8, 2016

Rewritten to use webapp/. Doesn't actually affect deploy script at all since we can still produce a tarball with a vector directory.

@dbkr dbkr assigned richvdh and unassigned dbkr Nov 8, 2016
/vector/config.json
/vector/index.html
/vector/olm.*
/vector
Copy link
Member

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/",
Copy link
Member

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?

Copy link
Member Author

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.

@richvdh richvdh assigned dbkr and unassigned richvdh Nov 8, 2016
@dbkr dbkr assigned richvdh and unassigned dbkr Nov 8, 2016
@richvdh
Copy link
Member

richvdh commented Nov 8, 2016

LGTM modulo update to jenkins.sh

@richvdh richvdh assigned dbkr and unassigned richvdh Nov 8, 2016
dbkr added 2 commits November 8, 2016 17:08
and make them re-use the package script rather than doing their
own thing
While we're breaking things anyway
@dbkr
Copy link
Member Author

dbkr commented Nov 8, 2016

Fixed - quick sanity check?

@dbkr dbkr assigned richvdh and unassigned dbkr Nov 8, 2016
Copy link
Member

@richvdh richvdh left a 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
Copy link
Member

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/ ?

Copy link
Member Author

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.

@richvdh richvdh assigned dbkr and unassigned richvdh Nov 8, 2016
@dbkr dbkr assigned richvdh and unassigned dbkr Nov 8, 2016
Copy link
Member

@richvdh richvdh left a 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

@richvdh richvdh assigned dbkr and unassigned richvdh Nov 10, 2016
@dbkr dbkr merged commit 2bcb27b into develop Nov 11, 2016
@dbkr dbkr deleted the dbkr/build_vector_dir branch December 14, 2016 18:09
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