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

Preprocessing step for ICU 59.1 on z (was: “some platforms”) #13304

Closed
srl295 opened this issue May 30, 2017 · 10 comments
Closed

Preprocessing step for ICU 59.1 on z (was: “some platforms”) #13304

srl295 opened this issue May 30, 2017 · 10 comments
Labels
i18n-api Issues and PRs related to the i18n implementation.

Comments

@srl295
Copy link
Member

srl295 commented May 30, 2017

Subsystem: deps

ICU 59.1 ( per #12486 ) needs a preprocessing step ( see here to convert u'文'; to u'\u6587';, because ICU's source code is now UTF-8.

The escaper itself is (currently) an ICU C++ tool, written using a subset of ICU's source code (only header files) that does not need any escaping or data. So it is a single .cpp file built into a single executable.

Platforms this is known to be needed on are:

  • IBM: AIX , z/OS (using xlC++)
  • Oracle: Solaris (using Oracle Studio 12.5 which is now the minimum supported version )
    edit this is only for z, see discussion

(if gcc or clang are used, there’s no issue.)

ICU4C handles all of this by fun with makefiles. I think for node’s build, it might be best done at configure time:

  • Actually compile the single binary
  • run all source code through to create the normal temporary node/deps/icu4c directory but with preprocessed source as well as original source
  • the temporary icu source is now compilable directly by the gyp-based toolchain without any additional issues.

the preprocessed source has #line directives in it so that compile errors, etc, show the original source names.

The escaper steps are basically:

  • escapesrc < somefile.cpp > _somefile.cpp
  • $(CC) -c _somefile.cpp …  (the usual build)
@mscdex mscdex added the i18n-api Issues and PRs related to the i18n implementation. label May 30, 2017
@bnoordhuis
Copy link
Member

Groan... the ICU build is already a twisty little maze of scripts and hacks all alike and now you want to add another step?

Platforms this is known to be needed on are:

IBM: AIX, z/OS (using xlC++)
Oracle: Solaris (using Oracle Studio 12.5 which is now the minimum supported version ).

Easy workaround: let them eat cake^W^Wuse gcc. We don't support those compilers.

I think for node’s build, it might be best done at configure time:

Strongly disagree. It might be acceptable when updating the in-tree copy but I'm not convinced it's necessary in the first place.

@refack
Copy link
Contributor

refack commented May 31, 2017

😭 #12218

It might be acceptable when updating the in-tree copy but I'm not convinced it's necessary in the first place.

👍 for floating this as a patch to the in tree copy (if necessary)

@gibfahn
Copy link
Member

gibfahn commented May 31, 2017

IBM: AIX

We use gcc on AIX (see the setup instructions).

Oracle: Solaris

Not a supported platform, and I'm pretty sure SmartOS uses gcc (cc/ @misterdjules)

IBM: z/OS (using xlC++)

cc/ @joransiu @jBarz @jbajwa re/ z/OS for whether the C++ compiler used for Node requires this.

@sam-github
Copy link
Contributor

I'm having trouble finding the escapesrc cpp source, couldn't it be done with a couple lines of python (already a build dep) instead of c++? And its not clear to me why the fixup isn't done before committing to github rather than on every single build, this is what @refack and others call "floating a patch", right?

@misterdjules
Copy link

@gibfahn

and I'm pretty sure SmartOS uses gcc (cc/ @misterdjules)

That's correct, GCC is used to build node on SmartOS.

@refack
Copy link
Contributor

refack commented May 31, 2017

I'm having trouble finding the escapesrc cpp source, couldn't it be done with a couple lines of python (already a build dep) instead of c++? And its not clear to me why the fixup isn't done before committing to github rather than on every single build, this is what @refack and others call "floating a patch", right?

👍 on both points.

@srl295
Copy link
Member Author

srl295 commented Jun 13, 2017

Groan... the ICU build is already a twisty little maze of scripts and hacks all alike and now you want to add another step?

At least they are more alike now… but yeah. 🙁

  • Re @misterdjules and @gibfahn : Glad to hear that Solaris and AIX should be good to go. 😌

  • that leaves z/OS?

Source code: escapesrc/

per @sam-github

And its not clear to me why the fixup isn't done before committing to github rather than on every single build

Because for most users, the code does not need to change. It seems like the fewer files that change between deps/ and upstream, the better. But i could go either way (if this is even needed).

In my implementation, I don't change any files in-place, but only cache altered copies right at the point of compilation. That won't go over well with gyp, perhaps.

couldn't it be done with a couple lines of python (already a build dep) instead of c++?

Yes, technically. That's an option.

And all, sorry to not put more context around this (and then to go on leave right after!) — I mainly wanted to make sure this was captured as a possible issue.

@srl295 srl295 changed the title Preprocessing step for ICU 59.1 on some platforms Preprocessing step for ICU 59.1 on z (was: “some platforms”) Jun 13, 2017
@joransiu
Copy link
Contributor

Re: z/OS -- There is no GCC on the platform. XL C/C++ compiler does not currently support unicode source or actual unicode in the source. To build ICU component on z/OS, we will need the preprocessor step that @srl295 mentioned.

@Trott
Copy link
Member

Trott commented Apr 30, 2018

@srl295 Should this remain open? If so, has anything changed that's worth noting?

@apapirovski
Copy link
Member

I'm going to close this out given the lack of a follow up since the last time @Trott resurrected this. Feel free to reopen if you've got something more current.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n-api Issues and PRs related to the i18n implementation.
Projects
None yet
Development

No branches or pull requests

10 participants