Skip to content

Report compile errors in shared library #327

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

Merged
merged 15 commits into from
Jul 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .github/workflows/linux.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,13 @@ jobs:
bundle exec ensure_arduino_installation.rb
sh ./scripts/install.sh
bundle exec arduino_ci.rb

SharedLibrary:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- uses: ruby/setup-ruby@v1
with:
ruby-version: 2.6
- name: Test SharedLibrary should fail
run: ./SampleProjects/SharedLibrary/test.sh
10 changes: 10 additions & 0 deletions .github/workflows/macos.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,13 @@ jobs:
bundle exec ensure_arduino_installation.rb
sh ./scripts/install.sh
bundle exec arduino_ci.rb

SharedLibrary:
runs-on: macos-latest
steps:
- uses: actions/checkout@v2
- uses: ruby/setup-ruby@v1
with:
ruby-version: 2.6
- name: Test SharedLibrary should fail
run: ./SampleProjects/SharedLibrary/test.sh
10 changes: 10 additions & 0 deletions .github/workflows/windows.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -50,3 +50,13 @@ jobs:
bundle exec ensure_arduino_installation.rb
bash -x ./scripts/install.sh
bundle exec arduino_ci.rb

SharedLibrary:
runs-on: windows-latest
steps:
- uses: actions/checkout@v2
- uses: ruby/setup-ruby@v1
with:
ruby-version: 2.6
- name: Test SharedLibrary should fail
run: ./SampleProjects/SharedLibrary/test.sh
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- Fix copy/paste error to allow additional warnings for a platform
- Apply "rule of three" to Client copy constructor and copy assignment operator
- Run Windows tests on Windows not Ubuntu
- Properly report error in building shared library

### Security

Expand Down
7 changes: 7 additions & 0 deletions SampleProjects/SharedLibrary/.arduino-ci.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
unittest:
platforms:
- mega2560

compile:
platforms:
- mega2560
1 change: 1 addition & 0 deletions SampleProjects/SharedLibrary/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
.bundle
2 changes: 2 additions & 0 deletions SampleProjects/SharedLibrary/Gemfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
source 'https://rubygems.org'
gem 'arduino_ci', path: '../../'
3 changes: 3 additions & 0 deletions SampleProjects/SharedLibrary/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# SharedLibrary

This is an example of a shared library with a compile error (see https://github.com/Arduino-CI/arduino_ci/issues/325).
10 changes: 10 additions & 0 deletions SampleProjects/SharedLibrary/library.properties
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
name=SharedLibrary
version=0.1.0
author=James Foster <arduino@jgfoster.net>
maintainer=James Foster <arduino@jgfoster.net>
sentence=Sample shared library to validate that we catch compile errors
paragraph=Sample shared library to validate that we catch compile errors
category=Other
url=https://github.com/Arduino-CI/arduino_ci/SampleProjects/SharedLibrary
architectures=avr,esp8266
includes=SharedLibrary.h
5 changes: 5 additions & 0 deletions SampleProjects/SharedLibrary/src/SharedLibrary.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#include "SharedLibrary.h"

int main() {
return foo; // 'foo' was not declared in this scope
}
3 changes: 3 additions & 0 deletions SampleProjects/SharedLibrary/src/SharedLibrary.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
#pragma once

#include <Arduino.h>
9 changes: 9 additions & 0 deletions SampleProjects/SharedLibrary/test.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
g++ -v
cd SampleProjects/SharedLibrary
bundle install
bundle exec ensure_arduino_installation.rb
bundle exec arduino_ci.rb --skip-examples-compilation
if [ $? -ne 1 ]; then
exit 1
fi
exit 0
13 changes: 13 additions & 0 deletions SampleProjects/SharedLibrary/test/test.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/*
cd SampleProjects/SharedLibrary
bundle config --local path vendor/bundle
bundle install
bundle exec arduino_ci.rb --skip-examples-compilation
*/

#include <Arduino.h>
#include <ArduinoUnitTests.h>

unittest(test) { assertEqual(true, true); }

unittest_main()
35 changes: 16 additions & 19 deletions exe/arduino_ci.rb
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,22 @@ def perform_unit_tests(cpp_library, file_config)
puts
compilers.each do |gcc_binary|
# before compiling the tests, build a shared library of everything except the test code
next unless build_shared_library(gcc_binary, p, config, cpp_library)
got_shared_library = true
attempt_multiline("Build shared library with #{gcc_binary} for #{p}") do
exe = cpp_library.build_shared_library(
config.aux_libraries_for_unittest,
gcc_binary,
config.gcc_config(p)
)
unless exe
puts "Last command: #{cpp_library.last_cmd}"
puts cpp_library.last_out
puts cpp_library.last_err
got_shared_library = false
end
next got_shared_library
end
next unless got_shared_library
Comment on lines +426 to +441
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain this change a bit? I'm not opposed to it, it just seems weird to move a small function into a big function

Copy link
Member Author

Choose a reason for hiding this comment

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

To be honest, I don't understand it myself. But the symptom was that a failure in the compile shared library function (which called the attempt_multiline() method) did not get properly reported out when it failed. That is, we got a compile error in the shared library then went on to compile the examples and when the examples compiled okay the overall test reported as passed. It seemed to me that when the attempt_multiline() method recognized a failure it reported it out one level but it didn't get out two levels. That is, by putting the call into a small, well-named method (which would otherwise be a good practice), it swallowed the error. But by moving it up a level the error got properly reported.

Of course, this is all speculation and since I don't really understand what is going on I'm not real comfortable with the explanation. In an case, I added tests that confirmed the problem and verified the fix. But I really would welcome a better understanding of what changed and how this fixed it!


# now build and run each test using the shared library build above
config.allowable_unittest_files(cpp_library.test_files).each do |unittest_path|
Expand All @@ -445,24 +460,6 @@ def perform_unit_tests(cpp_library, file_config)
end
end

def build_shared_library(gcc_binary, platform, config, cpp_library)
attempt_multiline("Build shared library with #{gcc_binary} for #{platform}") do
exe = cpp_library.build_shared_library(
config.aux_libraries_for_unittest,
gcc_binary,
config.gcc_config(platform)
)
puts
unless exe
puts "Last command: #{cpp_library.last_cmd}"
puts cpp_library.last_out
puts cpp_library.last_err
return false
end
return true
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jgfoster I think the actual problem was this return true which should just be an implicit return of exe 🤦 I'll push up this change in a bit and see what happens

end
end

def perform_example_compilation_tests(cpp_library, config)
phase("Compilation of example sketches")
if @cli_options[:skip_compilation]
Expand Down