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: fix RELEASE check (URGENT) #1421

Closed
wants to merge 1 commit into from
Closed

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Apr 14, 2015

Ref: #1405

please review @jbergstroem || @bnoordhuis asap or I'm just going to merge this and release a 1.7.1, 1.7.0 is a write-off because this is broken unfortunately.

@rvagg
Copy link
Member Author

rvagg commented Apr 14, 2015

for reference, the error on a release build is:

#NODE_VERSION_IS_RELEASE is set to (hell sed -ne 's/.
Did you remember to update src/node_version.h?

I was weary about this change and even went to the effort of testing the suggested sed change on a few platforms, pity I missed the syntax error in the process.

@rvagg
Copy link
Member Author

rvagg commented Apr 14, 2015

meh, merging without review

rvagg added a commit that referenced this pull request Apr 14, 2015
fixes broken 1.7.0 build, unreviewed quick patch

Ref: #1405
PR-URL: #1421
@rvagg
Copy link
Member Author

rvagg commented Apr 14, 2015

landed @ fa91b18

@rvagg rvagg closed this Apr 14, 2015
@jbergstroem
Copy link
Member

For what its worth: post-merge LGTM. Sorry about the slip-up.

@rvagg rvagg mentioned this pull request Apr 14, 2015
@rvagg
Copy link
Member Author

rvagg commented Apr 14, 2015

still no good, on OSX:

Makefile:200: *** unterminated call to function `shell': missing `)'.  Stop.

I don't even ...

Makefiles :shakesfist: and cross-platform sed :shakesfist:

rvagg added a commit to rvagg/io.js that referenced this pull request Apr 14, 2015
Notable changes:

* build: A syntax error in the Makefile for release builds caused
  1.7.0 to be DOA and unreleased. (Rod Vagg) nodejs#1421.
@jbergstroem
Copy link
Member

I can't reproduce. Tried on gnu make 3.81:

# cat foo 
all:
    RELEASE=$(shell sed -ne 's/#define NODE_VERSION_IS_RELEASE \([01]\)/\1/p' src/node_version.h)

# make -f foo
RELEASE=0

rvagg added a commit that referenced this pull request Apr 14, 2015
fixes broken 1.7.0 build, unreviewed quick patch

Ref: #1405
PR-URL: #1421
@rvagg
Copy link
Member Author

rvagg commented Apr 14, 2015

force-pushed another fix in this from

RELEASE=$(shell sed -ne 's/#define NODE_VERSION_IS_RELEASE \([01]\)/\1/p' src/node_version.h)

to

RELEASE=$(shell sed -ne 's/\#define NODE_VERSION_IS_RELEASE \([01]\)/\1/p' src/node_version.h)

it's now aee86a2

thanks @jbergstroem for spotting that as the problem

rvagg added a commit that referenced this pull request Apr 14, 2015
Notable changes:

* build: A syntax error in the Makefile for release builds caused
  1.7.0 to be DOA and unreleased. (Rod Vagg) #1421
jasnell added a commit to nodejs/dev-policy that referenced this pull request Apr 15, 2015
Add allowance for release time commits and error corrections.
Per: #48 (comment)
Example: nodejs/node#1421 and nodejs/node@fd90b33...50e9fc1.
dreamhigh0525 pushed a commit to dreamhigh0525/dev-policy that referenced this pull request Mar 9, 2023
Add allowance for release time commits and error corrections.
Per: nodejs/dev-policy#48 (comment)
Example: nodejs/node#1421 and nodejs/node@fd90b33...50e9fc1.
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