Skip to content

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

Merged
dbkr merged 9 commits into
developfrom
dbkr/build_vector_dir
Nov 11, 2016
Merged

Build all of the vector dir in the build process#2558
dbkr merged 9 commits into
developfrom
dbkr/build_vector_dir

Conversation

@dbkr

@dbkr dbkr commented Nov 8, 2016

Copy link
Copy Markdown
Member

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.

@richvdh richvdh left a comment

Copy link
Copy Markdown
Member

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?

Comment thread package.json Outdated
"build": "node scripts/babelcheck.js && npm run build:res && npm run build:config && npm run build:emojione && npm run build:css && npm run build:bundle",
"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
Copy Markdown
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
Copy Markdown
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

dbkr commented Nov 8, 2016

Copy link
Copy Markdown
Member Author

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

dbkr commented Nov 8, 2016

Copy link
Copy Markdown
Member Author

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
Comment thread .gitignore Outdated
/vector/config.json
/vector/index.html
/vector/olm.*
/vector

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

s/vector/webapp/

Comment thread package.json
"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
Copy Markdown
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
Copy Markdown
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

richvdh commented Nov 8, 2016

Copy link
Copy Markdown
Member

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

dbkr commented Nov 8, 2016

Copy link
Copy Markdown
Member Author

Fixed - quick sanity check?

@dbkr dbkr assigned richvdh and unassigned dbkr Nov 8, 2016

@richvdh richvdh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks plausible otherwise

Comment thread jenkins.sh Outdated

# 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
Copy Markdown
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
Copy Markdown
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

@richvdh richvdh left a comment

Copy link
Copy Markdown
Member

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
t3chguy pushed a commit that referenced this pull request Mar 25, 2026
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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