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

Generated test: add to make clean and fix removal from tarball #2633

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@rikardfalkeborn
Contributor

rikardfalkeborn commented Jun 4, 2018

From what I could read in the mentioned commit, it was intended to not include the generated code in the tarball (hence the dist-rule), but it doesn't work (I never bothered figure out why). The size difference of the tarball is 8kB so pretty much negligible.

Also, add it to make clean to add a simple way to remove accidental changes to the generated test.

@rikardfalkeborn

This comment has been minimized.

Show comment
Hide comment
@rikardfalkeborn

rikardfalkeborn Jun 5, 2018

Contributor

Apparently I didn't check the cmake build. Before I try to fix this, is the idea of the PR sound?

I think it should be possible to get it to work by modifying tests/libtest/CMakeLists.txt according to the following but I haven't manage to get the Cmake build work locally.

 foreach(TEST_NAME ${noinst_PROGRAMS})
-  setup_test(${TEST_NAME} ${${TEST_NAME}_SOURCES})
+  if(NOT DEFINED ${${TEST_NAME}_SOURCES})
+    setup_test(${TEST_NAME} ${nodist_${TEST_NAME}_SOURCES})
+  else()
+    setup_test(${TEST_NAME} ${${TEST_NAME}_SOURCES})
+  endif()
 endforeach()
Contributor

rikardfalkeborn commented Jun 5, 2018

Apparently I didn't check the cmake build. Before I try to fix this, is the idea of the PR sound?

I think it should be possible to get it to work by modifying tests/libtest/CMakeLists.txt according to the following but I haven't manage to get the Cmake build work locally.

 foreach(TEST_NAME ${noinst_PROGRAMS})
-  setup_test(${TEST_NAME} ${${TEST_NAME}_SOURCES})
+  if(NOT DEFINED ${${TEST_NAME}_SOURCES})
+    setup_test(${TEST_NAME} ${nodist_${TEST_NAME}_SOURCES})
+  else()
+    setup_test(${TEST_NAME} ${${TEST_NAME}_SOURCES})
+  endif()
 endforeach()

@bagder bagder added the build label Jun 5, 2018

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jun 5, 2018

Member

is the idea of the PR sound?

Absolutely!

Member

bagder commented Jun 5, 2018

is the idea of the PR sound?

Absolutely!

@rikardfalkeborn

This comment has been minimized.

Show comment
Hide comment
@rikardfalkeborn

rikardfalkeborn Jun 6, 2018

Contributor

New attempt. The only difference is that in this one I tried to fix the CMake build.

Contributor

rikardfalkeborn commented Jun 6, 2018

New attempt. The only difference is that in this one I tried to fix the CMake build.

rikardfalkeborn added some commits May 31, 2018

tests/libtests/Makefile.am: Add lib1521.c to CLEANFILES
This removes the generated lib1521.c when running make clean.
tests/libtest: Add lib1521 to nodist_SOURCES
Since 467da3a, lib1521.c is generated instead of checked in. According
to the commit message, the intention was to remove it from the tarball
as well. However, it is still present when running make dist. To remove
it, add it to nodist_lib1521_SOURCES. This also means there is no need
for the manually added dist-rule in the Makefile.

Also update CMakelists.txt to handle the fact that we now may have
nodist_SOURCES.
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jun 6, 2018

Member

Thanks!

Member

bagder commented Jun 6, 2018

Thanks!

@bagder bagder closed this in b59cbf7 Jun 6, 2018

@rikardfalkeborn rikardfalkeborn deleted the rikardfalkeborn:lib1521-makefile branch Jun 7, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Sep 5, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.