Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

fix: enable ltcg on windows #104

Merged
merged 2 commits into from
Jun 10, 2019
Merged

fix: enable ltcg on windows #104

merged 2 commits into from
Jun 10, 2019

Conversation

nornagon
Copy link
Member

@nornagon nornagon mentioned this pull request Jun 10, 2019
Closed
3 tasks
@nornagon nornagon requested a review from miniak June 10, 2019 16:22
refack
refack reviewed Jun 10, 2019
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

FWIW, I think the gist is correct.

tools/generate_config_gypi.py Outdated
with open(out, 'w') as f:
f.write("{'variables':{'built_with_electron': 1}}\n")
f.write(repr(config) + "\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I wouldn't use repr, but rather json.dumps...

Copy link
Member Author

Choose a reason for hiding this comment

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

it's not json :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Right you are...

node/configure.py

Lines 1602 to 1603 in 229bd32

write('config.gypi', do_not_edit +
pprint.pformat(output, indent=2) + '\n')

...

node/tools/gyp/pylib/gyp/input.py

Lines 240 to 243 in 229bd32

if check:
build_file_data = CheckedEval(build_file_contents)
else:
build_file_data = eval(build_file_contents, {'__builtins__': None},

But still... IMHO repr is less optimal then other options, and json is the most normative format that's compatible with GYP[3] ¯\(ツ)/¯

Copy link
Contributor

Choose a reason for hiding this comment

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

P.S. nodejs/node#25798

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, i didn't realise that gyp could read json! cool, will update.

@nornagon nornagon mentioned this pull request Jun 10, 2019
Closed
6 tasks
@zcbenz zcbenz merged commit aa79b3c into electron-node-v12.x Jun 10, 2019
@MarshallOfSound MarshallOfSound deleted the ltcg branch June 10, 2019 23:51
@nornagon
Copy link
Member Author

Apparently this doesn't actually fix the issue according to @miniak

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@refack refack refack left review comments

@miniak miniak miniak approved these changes

@zcbenz zcbenz zcbenz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nornagon @refack @zcbenz @miniak