Skip to content

Add Java output#605

Open
lynxize wants to merge 2 commits into
KhronosGroup:mainfrom
lynxize:java-output
Open

Add Java output#605
lynxize wants to merge 2 commits into
KhronosGroup:mainfrom
lynxize:java-output

Conversation

@lynxize

@lynxize lynxize commented May 25, 2026

Copy link
Copy Markdown

I don't know if there's any interest in including this, but I needed it for a project and figured there's no harm in opening a PR.

The output is Spv.java rather than spirv.java because Java requires public classes be declared in files that match their name.

@CLAassistant

CLAassistant commented May 25, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@bashbaug

Copy link
Copy Markdown
Contributor

Hi! Thank you for your contribution!

This all seems fine, but it's been a while since many of us have done anything with Java. We'll try to find a Java-savvy reviewer, but it might take a bit, so please be patient.

Thanks again!

@dgkoch

dgkoch commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Thanks for adding Java output here. I smoke-tested the generated Spv.java with a small Java app, and explicit compilation works correctly.

One Java packaging issue stood out: Spv.java declares:

package org.khronos.spv;

but the generated file is currently written to:

include/spirv/unified1/Spv.java

That means normal Java source discovery does not work, because javac expects the source path to match the package name. For example, treating include/spirv/unified1 as the source root fails with:

bad source file: .../include/spirv/unified1/Spv.java
file does not contain class Spv
Please remove or make sure it appears in the correct subdirectory of the sourcepath.

I think the best fix is to generate/check in the file at:

include/spirv/unified1/org/khronos/spv/Spv.java

and keep include/spirv/unified1 as the Java source root.

That would also require:

  • updating PrintAllHeaders() to create the parent directories before opening the output file;
  • changing the Java entry from Spv.java to org/khronos/spv/Spv.java;
  • updating makeHeaders / dos2unix handling for the nested file;
  • updating REUSE.toml for the new path.

The alternative would be to remove the package declaration, but I think keeping the package and moving the generated file under the matching directory is more useful for Java consumers.

@lynxize

lynxize commented Jun 4, 2026

Copy link
Copy Markdown
Author

My one concern with putting the file at include/spirv/unified1/org/khronos/spv/Spv.java is that every other language is directly inside include/spirv/unified1/ and personally I'm not sure that I'd think to check inside an org folder for a Java version. I guess I had assumed that users would place the file at the appropriate place within their project and/or change the package declaration as necessary, rather than using include/spirv/unified1 as a source root.

I do agree that moving the file is a better solution than removing the package declaration entirely, and I'll try to make the changes in the next couple of days.

@dgkoch

dgkoch commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

every other language is directly inside include/spirv/unified1/ and personally I'm not sure that I'd think to check inside an org folder for a Java version

(just to preface that it's been over 25 years since I've really done Java development - so my knowledge might be a bit out of date, but) I think that that's probably what Java developers would expect based on the package name, and there are breadcrumbs to that package name in the other headers that do end up in directly in the unified/ folder.

@lynxize

lynxize commented Jun 4, 2026

Copy link
Copy Markdown
Author

updating PrintAllHeaders() to create the parent directories before opening the output file

I don't usually write C++ so I may just be missing something obvious, but I (somewhat embarrassingly) can't figure out the best way create a directory. It seems like std::filesystem was introduced in C++ 17 and therefore can't be used here, and I'm hesitant to use platform-specific APIs because it looks like the rest of the code avoids them. Is there a preferred way to do this?

Instead I made the makeHeaders script create the directories itself, but that doesn't feel great because buildSpvHeaders will just fail if they don't exist.

@dgkoch

dgkoch commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Thanks for moving the Java file. I retested this shape locally, and the Java sourcepath issue is fixed now: treating include/spirv/unified1 as the source root lets javac find org.khronos.spv.Spv, and the smoke test passes.

On the directory creation question: I agree std::filesystem is not available if this tool is staying at C++11. I think the makeHeaders mkdir -p workaround is fine for the script, but since buildSpvHeaders -H is documented as generating all supported headers into the current directory, it would be better for the generator to create parent directories itself. Right now direct -H prints:

Unable to open file: org/khronos/spv/Spv.java

and exits successfully without generating the Java file.

A small localized C++11 helper seems reasonable here, even though it needs the usual _mkdir / mkdir split:

#include <cerrno>

#ifdef _WIN32
#include <direct.h>
#else
#include <sys/stat.h>
#include <sys/types.h>
#endif

bool makeDirectory(const std::string& path) {
#ifdef _WIN32
    const int result = _mkdir(path.c_str());
#else
    const int result = mkdir(path.c_str(), 0755);
#endif
    return result == 0 || errno == EEXIST;
}

bool ensureParentDirectories(const std::string& filename) {
    std::string::size_type pos = 0;
    while ((pos = filename.find(/, pos)) != std::string::npos) {
        const std::string path = filename.substr(0, pos);
        ++pos;
        if (!path.empty() && !makeDirectory(path))
            return false;
    }
    return true;
}

Then call ensureParentDirectories(lang.second) before opening the ofstream in PrintAllHeaders().

I tested this approach locally on macOS: direct buildSpvHeaders -H creates org/khronos/spv/Spv.java, the generated file matches the checked-in Java file, and the Java smoke test still passes.

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.

4 participants