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.

Sign up for GitHub

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

Jump to bottom

Internal modules problem with $ in identifiers on AIX #2272

Closed
mhdawson opened this issue Jul 29, 2015 · 3 comments
Closed

Internal modules problem with $ in identifiers on AIX #2272

mhdawson opened this issue Jul 29, 2015 · 3 comments
Labels
module Issues and PRs related to the module subsystem.

Comments

@mhdawson
Copy link
Member

Support for internal modules was added in:

2db758c

As part of this, this line in tools/utils.py converts a path name into an identifier

   escaped_id = id.replace('/', '$')

Unfortunately support for a $ in an identifier is implementation specific and is not supported on AIX which causes compiler failures on the generated node_natives.h file.

I'd propose to change the line to:

  escaped_id = id.replace('/', '_')

@vkurchatkin comments ?
@brendanashworth brendanashworth added the module Issues and PRs related to the module subsystem. label Jul 29, 2015
@vkurchatkin
Copy link
Contributor

I think it's fine. I chose $ instead of _ in the first place to avoid possible collisions, it's rather unlikely that we will have $ in paths, but in generally it's not a big deal, I think.

@Fishrock123 Fishrock123 changed the title Internal modules problem with $ in identifiers Internal modules problem with $ in identifiers on AIX Jul 30, 2015
@mhdawson
Copy link
Member Author

Thanks, I'll include that change when I submit the PR to add AIX support

@mhdawson mhdawson mentioned this issue Aug 12, 2015
Closed
@brendanashworth
Copy link
Contributor

I think this can be closed for now, with discussion continuing on #2364. It can always be reopened, of course.

mhdawson added a commit to mhdawson/io.js that referenced this issue Sep 15, 2015
@mhdawson
build: Updates to enable AIX support
5a19293 
These are the core changes that allow AIX to compile.  There
are still some test failures as there are some patches needed for
libuv and npm that we'll need to contribute through those
communities but this set allows node to be built on AIX and
pass most of the core tests

The change in js2c is because AIX does not support $ in
identifier names.  See the discussion/agreement in
nodejs#2272

PR-URL: nodejs#2364
Reviewed-By: Ben Noordhuis <ben@strongloop.com>
Reviewed-By: Rod Vagg <r@va.gg>
mhdawson added a commit that referenced this issue Sep 15, 2015
@mhdawson
build: Updates to enable AIX support
2a17c7f 
These are the core changes that allow AIX to compile.  There
are still some test failures as there are some patches needed for
libuv and npm that we'll need to contribute through those
communities but this set allows node to be built on AIX and
pass most of the core tests

The change in js2c is because AIX does not support $ in
identifier names.  See the discussion/agreement in
#2272

PR-URL: #2364
Reviewed-By: Ben Noordhuis <ben@strongloop.com>
Reviewed-By: Rod Vagg <r@va.gg>
mhdawson added a commit that referenced this issue Sep 15, 2015
@mhdawson @Fishrock123
build: Updates to enable AIX support
43397b2 
These are the core changes that allow AIX to compile.  There
are still some test failures as there are some patches needed for
libuv and npm that we'll need to contribute through those
communities but this set allows node to be built on AIX and
pass most of the core tests

The change in js2c is because AIX does not support $ in
identifier names.  See the discussion/agreement in
#2272

PR-URL: #2364
Reviewed-By: Ben Noordhuis <ben@strongloop.com>
Reviewed-By: Rod Vagg <r@va.gg>
@snyk-bot snyk-bot mentioned this issue Aug 11, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
module Issues and PRs related to the module subsystem.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@vkurchatkin @brendanashworth @mhdawson