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

Reduce tarball size, v2 #5695

Merged
merged 1 commit into from
Mar 16, 2016

Conversation

jbergstroem
Copy link
Member

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

build, deps, tools

Description of change

Reduce the nodejs tarball size with another ~10% (it's grown pretty fast over the last year). The distinction we're making here is that developers should opt for git instead of tarballs, but the test suite should still be able to run (therefore verifying the generated binary). This means that stuff like linters or test suites in deps/ should go.

@jbergstroem jbergstroem added the build Issues and PRs related to build files or the CI. label Mar 14, 2016
@jbergstroem
Copy link
Member Author

CC: @nodejs/build

CI: https://ci.nodejs.org/job/node-test-commit/2532/

@mscdex
Copy link
Contributor

mscdex commented Mar 14, 2016

Do the linting-related files really take up that much space though compared to the other things being removed in this PR?

@jbergstroem
Copy link
Member Author

@mscdex 8% tarball size (9% xz).

@ChALkeR
Copy link
Member

ChALkeR commented Mar 14, 2016

Should this be labeled semver-major?

@jbergstroem
Copy link
Member Author

@ChALkeR because we're removing eslint from the tarball? Good question. I'm not against it.

@rvagg rvagg added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 14, 2016
@@ -261,6 +261,7 @@ goto jslint

:jslint
if not defined jslint goto exit
if not exist tools\eslint\bin\eslint.js goto exit
Copy link
Member

Choose a reason for hiding this comment

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

thinking you should probably be as nice here as you are in the Makefile about what's wrong

@rvagg
Copy link
Member

rvagg commented Mar 14, 2016

semver-major is a good idea since it's a change in expectations for users building from source

lgtm

else
test-v8 test-v8-intl test-v8-benchmarks test-v8-all:
@echo "Testing v8 is not available through the source tarball."
@echo "Use the git repo instead: $ git clone https://github.com/nodejs/node.git"
Copy link
Member

Choose a reason for hiding this comment

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

Long line?

@thefourtheye
Copy link
Contributor

LGTM.

@jasnell
Copy link
Member

jasnell commented Mar 14, 2016

LGTM with @bnoordhuis' comments addressed.

@@ -583,7 +593,7 @@ bench-idle:

jslint:
$(NODE) tools/eslint/bin/eslint.js benchmark lib src test tools/doc \
tools/eslint-rules --rulesdir tools/eslint-rules
tools/eslint-rules --rulesdir tools/eslint-rules
Copy link
Contributor

Choose a reason for hiding this comment

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

@Trott I think you committed these leading spaces, which can be problematic in a Makefile, thought it seems it didn't matter here. This change here could've been avoided with a Editorconfig plugin in your editor 😉

This removes the ability to run linting from the source tarball,
rationale being that developers should use a git clone instead.

Also, fix the path of removing artifacts from the openssl dependency
since it now lives in `deps/openssl/openssl/`.

Tarballs shrink with ~10%.

PR-URL: nodejs#5695
Fixes: nodejs#5618
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: thefourtheye <thechargingvolcano@gmail.com>
Reviewed-By: James Snell <jasnell@gmail.com>
@jbergstroem jbergstroem merged commit 90a5fc2 into nodejs:master Mar 16, 2016
@jasnell jasnell mentioned this pull request Mar 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants