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

Add tags/make releases for this repo #34

Open
christgau opened this issue Dec 5, 2023 · 8 comments
Open

Add tags/make releases for this repo #34

christgau opened this issue Dec 5, 2023 · 8 comments
Assignees

Comments

@christgau
Copy link

It would very helpful to know which API or loader version these tests require. The prerequisites in the BUILD file appear to be outdates as the file has not been updated for about two years. At the moment one can only guess which version of the loader/API these tests are working with (see #33). Therefore, I propose to introduce tags or make releases which may follow the same numbering scheme as the loader or the API (I don't care in the end, but it should be documented). At least the BUILD file should be updated.

@lisanna-dettwyler
Copy link
Contributor

We'll soon start adding conditional guards around test content that requires newer header versions which should solve this issue. I'll take a look at the BUILD file.

@eero-t
Copy link

eero-t commented Apr 16, 2024

We'll soon start adding conditional guards around test content that requires newer header versions which should solve this issue. I'll take a look at the BUILD file.

@lisanna-dettwyler, you should really re-open this as:

  • Few header guards are not the same thing as git tags and projects releases
  • Bug should really be closed only when it has been actually fixed in this repo (not in some random internal one), and reporter has verified it to be fixed (or several weeks have passed without reporter response)

@lisanna-dettwyler
Copy link
Contributor

This was closed inadvertently because an internal PR linked against it. GitHub closed it automatically when it was merged. I intend to keep this open until the BUILD.md file is updated publicly. We're not going to do releases for these tests, always use HEAD.

@eero-t
Copy link

eero-t commented Apr 16, 2024

We're not going to do releases for these tests, always use HEAD.

While releases would be nice, this bug is not requesting them, project tags would be enough.

Documentation can state what can be expected from the tagged commits; did they go through some additional testing, or do they indicate some change within the project, such as change in the required L0 spec version.

Minimal quality statement could be something like following:

Tagged commits do not have any guarantee of functionality. They have just been tested to successfully build with the L0 spec version indicated by the tag name. Tags follow the "builds-with-L0-spec-VERSION" format.

Tags are trivial to add, also for historical commits. And if tagged version eventually turns out to be "bad", tag can be removed.

PS. It would be better to avoid moving tags to different commits. If e.g. builds-with-L0-spec-v1.5 tag is found to point to a bad commit, e.g. new builds-with-L0-spec-v1.5-update1 tag can be added for a better one.

@christgau
Copy link
Author

We're not going to do releases for these tests, always use HEAD.

While releases would be nice, this bug is not requesting them, project tags would be enough.

It's not about releases for the tests, but pointing at HEAD alone does not help a user - as I'm such one. The scenario I faced was like this: Suppose you have a installation of the L0 runtime made by system administrators. How do I know which commit of the tests in this repo I can use to successfully compile. That's my main point.

Documentation can state what can be expected from the tagged commits; did they go through some additional testing, or do they indicate some change within the project, such as change in the required L0 spec version.

That would be fine as well.

Tags are trivial to add, also for historical commits.

2x Exactly. The ecosystem would benefit quite bit from this.

@Jemale Jemale closed this as completed in e94215d Apr 17, 2024
@Jemale Jemale reopened this Apr 17, 2024
@christgau
Copy link
Author

Thanks for updating the BUILD file. However, it still does not solve the issue:

The HEAD of this repo is compatible with the latest Level Zero Loader release. Please report incompatibilities at https://github.com/oneapi-src/level-zero-tests/issues.
You can either build against the version you have installed on your system (automatic, Linux only),
or specify an install prefix with the `CMAKE_PREFIX_PATH` cmake flag during configuration.

The first sentence is still rather imprecise. Is it guaranteed that HEAD of this repo and HEAD of the loader repo are always compatible? Or put another way: If I go back in time, because I need a specific version... uhm... commit, can I just pick the commit from the loader repo with the same date and it works?

And: What's the proposed way to identify the commit of this repo that works with a specific version of the loader?

Further, the second sentence from the BUILD file does not apply. Here's what I did following the steps fro the BUILD file:

...
$ cmake -D CMAKE_INSTALL_PREFIX=$PWD/../out ..
-- The C compiler identification is GNU 13.2.1
-- The CXX compiler identification is GNU 13.2.1
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Warning at cmake/clang_tools.cmake:20 (message):
  clang-format-7 not found.  clang-format and clang-format-check targets will
  be disabled.
Call Stack (most recent call first):
  CMakeLists.txt:12 (include)


-- Found ZLIB: /usr/lib/libz.so (found version "1.3.1")
-- Found PNG: /usr/lib/libpng.so (found version "1.6.43")
-- Found Git: /usr/bin/git (found version "2.44.0")
CMake Warning at CMakeLists.txt:26 (message):
  Unable to locate third_party/googletest submodule in checkout - Trying to
  fetch with /usr/bin/git submodule update --init --recursive


Submodule 'third_party/googletest' (https://github.com/google/googletest.git) registered for path 'third_party/googletest'
Cloning into '/tmp/level-zero-tests/third_party/googletest'...
Submodule path 'third_party/googletest': checked out '15460959cbbfa20e66ef0b5ab497367e47fc0a04'
-- Found Python: /usr/bin/python3.11 (found version "3.11.8") found components: Interpreter
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE
-- Found Boost: /usr/lib/cmake/Boost-1.83.0/BoostConfig.cmake (found suitable version "1.83.0", minimum required is "1.65") found components: log program_options timer chrono system serialization
-- Boost_FOUND: TRUE
-- Found LevelZero: /usr/include
-- LevelZero_LIBRARIES: /usr/lib/libze_loader.sodl
-- LevelZero_INCLUDE_DIRS: /usr/include
-- Found PAPI library: papi
[...]
-- Configuring done (3.6s)
-- Generating done (0.3s)
-- Build files have been written to: /tmp/level-zero-tests/build

Note that the cmake step finds my local installation which is great. Following up, I try to compile..

/tmp/level-zero-tests/build $ cmake --build . --config Release --target install
[ 93%] Built target test_api_ltracing
[ 93%] Building CXX object layer_tests/tracing/CMakeFiles/test_api_ltracing_dynamic.dir/src/test_api_ltracing_compat.cpp.o
[ 93%] Building CXX object layer_tests/tracing/CMakeFiles/test_api_ltracing_dynamic.dir/src/test_api_ltracing.cpp.o
[ 93%] Building CXX object layer_tests/tracing/CMakeFiles/test_api_ltracing_dynamic.dir/src/test_api_ltracing_threading.cpp.o
[ 93%] Building CXX object layer_tests/tracing/CMakeFiles/test_api_ltracing_dynamic.dir/src/main.cpp.o
/tmp/level-zero-tests/layer_tests/tracing/src/main.cpp: In function ‘int main(int, char**)’:
/tmp/level-zero-tests/layer_tests/tracing/src/main.cpp:29:3: error: ‘zelEnableTracingLayer’ was not declared in this scope
   29 |   zelEnableTracingLayer();
      |   ^~~~~~~~~~~~~~~~~~~~~
/tmp/level-zero-tests/layer_tests/tracing/src/main.cpp:35:3: error: ‘zelDisableTracingLayer’ was not declared in this scope
   35 |   zelDisableTracingLayer();
      |   ^~~~~~~~~~~~~~~~~~~~~~
make[2]: *** [layer_tests/tracing/CMakeFiles/test_api_ltracing_dynamic.dir/build.make:118: layer_tests/tracing/CMakeFiles/test_api_ltracing_dynamic.dir/src/main.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:5512: layer_tests/tracing/CMakeFiles/test_api_ltracing_dynamic.dir/all] Error 2

Ok, error missing declaration. Is that an error inside HEAD (would be bad) or are the header files installed on my maschine not compatible with the test repo (contradicts the sentence from the BUILD file)?

$ pacman -Ss level-zero
extra/intel-compute-runtime 23.48.27912.11-2 [installed]
    Intel(R) Graphics Compute Runtime for oneAPI Level Zero and OpenCL(TM) Driver
extra/level-zero-headers 1.15.1-1 [installed]
    API for accessing low level interfaces in oneAPI platform devices (headers)
extra/level-zero-loader 1.15.1-1 [installed]
    API for accessing low level interfaces in oneAPI platform devices (loader)

My proposal for the tags would be that I can do git checkout l0-loader-1.15.1-1 on a clone of this repo and be sure that it works/compiles with the version of loader/headers installed on my machine.

@lisanna-dettwyler
Copy link
Contributor

And: What's the proposed way to identify the commit of this repo that works with a specific version of the loader?

It's HEAD of master, the tests are intended to remain backwards-compatible with older releases. I'll make this clearer in the documentation, and add a section at the top talking about header compatibility. Historically this hasn't been the case, but we're going to start enforcing it.

Ok, error missing declaration. Is that an error inside HEAD (would be bad) or are the header files installed on my maschine not compatible with the test repo (contradicts the sentence from the BUILD file)?

The most recent version of the BUILD file addresses this, it appears you read the version just prior: https://github.com/oneapi-src/level-zero-tests/blob/master/BUILD.md?plain=1#L36

lisanna-dettwyler added a commit to lisanna-dettwyler/level-zero-tests that referenced this issue May 14, 2024
@lisanna-dettwyler
Copy link
Contributor

Version guards will only work for 1.9 onwards, since a required macro in the headers is set to be released in 1.10. In other words, loaders with at least 1.9 of the headers will be able to use HEAD of master indefinitely. I have created https://github.com/oneapi-src/level-zero-tests/releases/tag/spec-1.8 and https://github.com/oneapi-src/level-zero-tests/releases/tag/spec-1.7 for older loaders.

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

No branches or pull requests

4 participants