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

Rename VERSION to CPS_VERSION #91

Merged
merged 3 commits into from
Jul 19, 2024

Conversation

ckyang0225
Copy link
Contributor

@ckyang0225 ckyang0225 commented Jul 19, 2024

The VERSION file introduced from cc40978 caused meson build fail.
This PR fixed it by renaming VERSION file to CPS_VERSION.
Also verified all tests passed now, and CI passes, too.

Before:

ninja: Entering directory `builddir'
[1/16] Compiling C++ object src/libcps.a.p/cps_env.cpp.o
[2/16] Compiling C++ object loader_test.p/tests_loader.cpp.o
FAILED: loader_test.p/tests_loader.cpp.o
c++ -Iloader_test.p -I. -I.. -Isrc -I../src -I/opt/homebrew/Cellar/googletest/1.14.0/include -I/opt/homebrew/Cellar/fmt/10.2.1_1/include -I/opt/homebrew/include -I/opt/homebrew/share -fdiagnostics-color=always -D_GLIBCXX_ASSERTIONS=1 -D_LIBCPP_ENABLE_ASSERTIONS=1 -Wall -Winvalid-pch -Wextra -std=c++17 -O2 -g -DGTEST_HAS_PTHREAD=1 -MD -MQ loader_test.p/tests_loader.cpp.o -MF loader_test.p/tests_loader.cpp.o.d -o loader_test.p/tests_loader.cpp.o -c ../tests/loader.cpp
In file included from ../tests/loader.cpp:6:
In file included from ../src/cps/loader.hpp:7:
In file included from ../src/cps/version.hpp:7:
In file included from /opt/homebrew/include/tl/expected.hpp:23:
In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/exception:82:
In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/__exception/exception_ptr.h:13:
In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/__exception/operations.h:14:
In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/cstddef:41:
../version:1:1: error: expected unqualified-id
0.0.1
^

After:

ninja: Entering directory `builddir'
[6/16] Compiling C++ object loader_test.p/tests_loader.cpp.o
In file included from ../tests/loader.cpp:7:
/opt/homebrew/Cellar/googletest/1.14.0/include/gtest/gtest.h:1379:11: warning: comparison of integers of different signs: 'const unsigned long' and 'const int' [-Wsign-compare]
  if (lhs == rhs) {
      ~~~ ^  ~~~
/opt/homebrew/Cellar/googletest/1.14.0/include/gtest/gtest.h:1398:12: note: in instantiation of function template specialization 'testing::internal::CmpHelperEQ<unsigned long, int>' requested here
    return CmpHelperEQ(lhs_expression, rhs_expression, lhs, rhs);
           ^
../tests/loader.cpp:239:13: note: in instantiation of function template specialization 'testing::internal::EqHelper::Compare<unsigned long, int, nullptr>' requested here
            ASSERT_EQ(package->components.size(), 7) << "should have found 7 different components";
            ^
/opt/homebrew/Cellar/googletest/1.14.0/include/gtest/gtest.h:1898:31: note: expanded from macro 'ASSERT_EQ'
#define ASSERT_EQ(val1, val2) GTEST_ASSERT_EQ(val1, val2)
                              ^
/opt/homebrew/Cellar/googletest/1.14.0/include/gtest/gtest.h:1882:54: note: expanded from macro 'GTEST_ASSERT_EQ'
  ASSERT_PRED_FORMAT2(::testing::internal::EqHelper::Compare, val1, val2)
                                                     ^
1 warning generated.
[15/16] Running all tests.
1/5 loader                          OK              0.21s
2/5 version                         OK              0.35s
3/5 utils                           OK              0.52s
4/5 pkg-config compatibility        OK              0.74s   9 subtests passed
5/5 cps integration tests           OK              0.75s   20 subtests passed

Ok:                 5
Expected Fail:      0
Fail:               0
Unexpected Pass:    0
Skipped:            0
Timeout:            0

@ckyang0225
Copy link
Contributor Author

@bretbrownjr @lunacd @dcbaker Please take a look, thanks

Copy link
Collaborator

@bretbrownjr bretbrownjr left a comment

Choose a reason for hiding this comment

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

I like any change that makes automation more reliable, so I'd like to see this PR land.

However, I think "CPS_VERSION" as a filename might be confusing. That's very similar to the field in cps files that represents the version of the CPS schema (example). I think the file being changed here is supposed to represent the version of this project, which is a different thing.

Maybe name the file PROJECT_VERSION or CPS_CONFIG_VERSION instead?

@ckyang0225 or @dcbaker... can someone educate me about the filename VERSION confuses Meson?

@ckyang0225 ckyang0225 requested a review from bretbrownjr July 19, 2024 15:54
@ckyang0225
Copy link
Contributor Author

@ckyang0225 or @dcbaker... can someone educate me about the filename VERSION confuses Meson?

tbh I'm not meson expert but just observed this issue and tried to fix.
@dcbaker might answer it better I believe.

Copy link
Collaborator

@bretbrownjr bretbrownjr left a comment

Choose a reason for hiding this comment

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

It fixes things, so I'll merge. Happy to revisit with @dcbaker if we missed something.

@bretbrownjr bretbrownjr merged commit 6a54180 into cps-org:main Jul 19, 2024
8 checks passed
@ckyang0225 ckyang0225 deleted the cyang571-renameversionfile branch July 19, 2024 16:07
@dcbaker
Copy link
Collaborator

dcbaker commented Jul 19, 2024

Ah, I have an idea of what's going on. Meson by default includes the local directory, both source and build. I'll send out a followup to see if that fixes things.

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.

3 participants