From fc9d4d9738946770f3b7ea10813414343d7509fb Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Wed, 18 May 2016 18:42:53 +0200 Subject: [PATCH 1/2] Revert fix for #37 and always show "multiple libraries warning" again This reverts commit 20584795c86c40be9073ded245264f7a6becf881: Adding info in LibraryResolutionResult about whether selected library comes from a platform or from outside. If the former and there are duplicates, no warning is printed Additionally, the librariesInSomePlatform function and librariesInPlatforms variable in resolveLibrary are removed, since these are now unused. The original suggestion in #37 was that if a platform library overrides a builtin library, no warning would be needed. The implementation, however, was hiding the warning whenever a platform library was overriding any other library, including a user library, which seems harmful (This was [reported on the devlist][1]). This commit reverts the exception added for #37, making the warning appear unconditionally again. In a future commit, a better solution for #37 will be added. [1]: https://groups.google.com/a/arduino.cc/d/msg/developers/1kkIqIsbuzU/0-abwr1gBQAJ Signed-off-by: Matthijs Kooijman --- .../builder/includes_to_include_folders.go | 16 +--------- .../print_used_and_not_used_libraries.go | 10 +++---- .../test/includes_to_include_folders_test.go | 29 ------------------- src/arduino.cc/builder/types/types.go | 1 - 4 files changed, 5 insertions(+), 51 deletions(-) diff --git a/src/arduino.cc/builder/includes_to_include_folders.go b/src/arduino.cc/builder/includes_to_include_folders.go index 63c76f7f..4ddd7869 100644 --- a/src/arduino.cc/builder/includes_to_include_folders.go +++ b/src/arduino.cc/builder/includes_to_include_folders.go @@ -123,8 +123,6 @@ func resolveLibrary(header string, headerToLibraries map[string][]*types.Library reverse(libraries) - librariesInPlatforms := librariesInSomePlatform(libraries, platforms) - var library *types.Library for _, platform := range platforms { @@ -149,8 +147,7 @@ func resolveLibrary(header string, headerToLibraries map[string][]*types.Library library = useAlreadyImportedLibraryWithSameNameIfExists(library, markImportedLibrary) - isLibraryFromPlatform := findLibraryIn(librariesInPlatforms, library) != nil - libraryResolutionResults[header] = types.LibraryResolutionResult{Library: library, IsLibraryFromPlatform: isLibraryFromPlatform, NotUsedLibraries: filterOutLibraryFrom(libraries, library)} + libraryResolutionResults[header] = types.LibraryResolutionResult{Library: library, NotUsedLibraries: filterOutLibraryFrom(libraries, library)} markImportedLibrary[library] = true } @@ -162,17 +159,6 @@ func reverse(data []*types.Library) { } } -func librariesInSomePlatform(libraries []*types.Library, platforms []*types.Platform) []*types.Library { - librariesInPlatforms := []*types.Library{} - for _, platform := range platforms { - if platform != nil { - librariesWithinSpecifiedPlatform := librariesWithinPlatform(libraries, platform) - librariesInPlatforms = append(librariesInPlatforms, librariesWithinSpecifiedPlatform...) - } - } - return librariesInPlatforms -} - func markImportedLibraryContainsOneOfCandidates(markImportedLibrary map[*types.Library]bool, libraries []*types.Library) bool { for markedLibrary, _ := range markImportedLibrary { for _, library := range libraries { diff --git a/src/arduino.cc/builder/print_used_and_not_used_libraries.go b/src/arduino.cc/builder/print_used_and_not_used_libraries.go index ae490a9f..90df1a72 100644 --- a/src/arduino.cc/builder/print_used_and_not_used_libraries.go +++ b/src/arduino.cc/builder/print_used_and_not_used_libraries.go @@ -47,12 +47,10 @@ func (s *PrintUsedAndNotUsedLibraries) Run(ctx *types.Context) error { libraryResolutionResults := ctx.LibrariesResolutionResults for header, libResResult := range libraryResolutionResults { - if !libResResult.IsLibraryFromPlatform { - logger.Fprintln(os.Stdout, constants.LOG_LEVEL_WARN, constants.MSG_LIBRARIES_MULTIPLE_LIBS_FOUND_FOR, header) - logger.Fprintln(os.Stdout, constants.LOG_LEVEL_WARN, constants.MSG_LIBRARIES_USED, libResResult.Library.Folder) - for _, notUsedLibrary := range libResResult.NotUsedLibraries { - logger.Fprintln(os.Stdout, constants.LOG_LEVEL_WARN, constants.MSG_LIBRARIES_NOT_USED, notUsedLibrary.Folder) - } + logger.Fprintln(os.Stdout, constants.LOG_LEVEL_WARN, constants.MSG_LIBRARIES_MULTIPLE_LIBS_FOUND_FOR, header) + logger.Fprintln(os.Stdout, constants.LOG_LEVEL_WARN, constants.MSG_LIBRARIES_USED, libResResult.Library.Folder) + for _, notUsedLibrary := range libResResult.NotUsedLibraries { + logger.Fprintln(os.Stdout, constants.LOG_LEVEL_WARN, constants.MSG_LIBRARIES_NOT_USED, notUsedLibrary.Folder) } } diff --git a/src/arduino.cc/builder/test/includes_to_include_folders_test.go b/src/arduino.cc/builder/test/includes_to_include_folders_test.go index a0fb431f..ee3f19a9 100644 --- a/src/arduino.cc/builder/test/includes_to_include_folders_test.go +++ b/src/arduino.cc/builder/test/includes_to_include_folders_test.go @@ -73,10 +73,6 @@ func TestIncludesToIncludeFolders(t *testing.T) { importedLibraries := ctx.ImportedLibraries require.Equal(t, 1, len(importedLibraries)) require.Equal(t, "Bridge", importedLibraries[0].Name) - - libraryResolutionResults := ctx.LibrariesResolutionResults - require.NotNil(t, libraryResolutionResults) - require.False(t, libraryResolutionResults["Bridge.h"].IsLibraryFromPlatform) } func TestIncludesToIncludeFoldersSketchWithIfDef(t *testing.T) { @@ -112,9 +108,6 @@ func TestIncludesToIncludeFoldersSketchWithIfDef(t *testing.T) { importedLibraries := ctx.ImportedLibraries require.Equal(t, 0, len(importedLibraries)) - - libraryResolutionResults := ctx.LibrariesResolutionResults - require.NotNil(t, libraryResolutionResults) } func TestIncludesToIncludeFoldersIRremoteLibrary(t *testing.T) { @@ -153,11 +146,6 @@ func TestIncludesToIncludeFoldersIRremoteLibrary(t *testing.T) { require.Equal(t, 2, len(importedLibraries)) require.Equal(t, "Bridge", importedLibraries[0].Name) require.Equal(t, "IRremote", importedLibraries[1].Name) - - libraryResolutionResults := ctx.LibrariesResolutionResults - require.NotNil(t, libraryResolutionResults) - require.False(t, libraryResolutionResults["Bridge.h"].IsLibraryFromPlatform) - require.False(t, libraryResolutionResults["IRremote.h"].IsLibraryFromPlatform) } func TestIncludesToIncludeFoldersANewLibrary(t *testing.T) { @@ -196,11 +184,6 @@ func TestIncludesToIncludeFoldersANewLibrary(t *testing.T) { require.Equal(t, 2, len(importedLibraries)) require.Equal(t, "ANewLibrary-master", importedLibraries[0].Name) require.Equal(t, "IRremote", importedLibraries[1].Name) - - libraryResolutionResults := ctx.LibrariesResolutionResults - require.NotNil(t, libraryResolutionResults) - require.False(t, libraryResolutionResults["anewlibrary.h"].IsLibraryFromPlatform) - require.False(t, libraryResolutionResults["IRremote.h"].IsLibraryFromPlatform) } func TestIncludesToIncludeFoldersDuplicateLibs(t *testing.T) { @@ -238,10 +221,6 @@ func TestIncludesToIncludeFoldersDuplicateLibs(t *testing.T) { require.Equal(t, 1, len(importedLibraries)) require.Equal(t, "SPI", importedLibraries[0].Name) require.Equal(t, Abs(t, filepath.Join("user_hardware", "my_avr_platform", "avr", "libraries", "SPI")), importedLibraries[0].SrcFolder) - - libraryResolutionResults := ctx.LibrariesResolutionResults - require.NotNil(t, libraryResolutionResults) - require.True(t, libraryResolutionResults["SPI.h"].IsLibraryFromPlatform) } func TestIncludesToIncludeFoldersDuplicateLibsWithConflictingLibsOutsideOfPlatform(t *testing.T) { @@ -280,10 +259,6 @@ func TestIncludesToIncludeFoldersDuplicateLibsWithConflictingLibsOutsideOfPlatfo require.Equal(t, 1, len(importedLibraries)) require.Equal(t, "SPI", importedLibraries[0].Name) require.Equal(t, Abs(t, filepath.Join("libraries", "SPI")), importedLibraries[0].SrcFolder) - - libraryResolutionResults := ctx.LibrariesResolutionResults - require.NotNil(t, libraryResolutionResults) - require.False(t, libraryResolutionResults["SPI.h"].IsLibraryFromPlatform) } func TestIncludesToIncludeFoldersDuplicateLibs2(t *testing.T) { @@ -322,8 +297,4 @@ func TestIncludesToIncludeFoldersDuplicateLibs2(t *testing.T) { require.Equal(t, 1, len(importedLibraries)) require.Equal(t, "USBHost", importedLibraries[0].Name) require.Equal(t, Abs(t, filepath.Join("libraries", "USBHost", "src")), importedLibraries[0].SrcFolder) - - libraryResolutionResults := ctx.LibrariesResolutionResults - require.NotNil(t, libraryResolutionResults) - require.False(t, libraryResolutionResults["Usb.h"].IsLibraryFromPlatform) } diff --git a/src/arduino.cc/builder/types/types.go b/src/arduino.cc/builder/types/types.go index b9750197..f9723a62 100644 --- a/src/arduino.cc/builder/types/types.go +++ b/src/arduino.cc/builder/types/types.go @@ -174,7 +174,6 @@ type SourceFolder struct { type LibraryResolutionResult struct { Library *Library - IsLibraryFromPlatform bool NotUsedLibraries []*Library } From 31f1d5520cc80c4b3ddd9462d8a6eae6ee52278c Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Wed, 18 May 2016 18:58:57 +0200 Subject: [PATCH 2/2] Do not always print multiple libraries warning Previously, this warning was always printed when multiple libraries were found for a header filename. #37 reported that this can be confusing for novice users, when for example a platform library overrides a builtin library, which is typically considered "normal" and should not generate a warning. The previous attempt at fixing #37 (which was reverted) would hide the warning in these "normal" cases, but it ended up also hiding the message in cases where it would be relevant (as [reported on the mailing list][1]). Also, if for a single compilation some of these messages are hidden, but others are not, this might cause users to draw incorrect conclusions if they are not aware of the exceptions ("there must be only one library for this header file, since I didn't see any message"). This commit takes a different approach: these messages are only shown when: - The sketch fails to compile. In this case, the message is emitted as a warning, shown in red in the IDE. - Verbose compilation is enabled. In this case, the message is emitted as informational, shown in white in the IDE. This means that these message are either *all* shown, or *none* of them are shown, which should make the behaviour more predictable. There is still a chance that the "normal" messages described in #37 confuse a novice user when there is a compilation error completely unrelated to the chosen libraries, but it is pretty impossible for arduino-builder to know the cause of a failed compilation, so just showing all of these messages on an error seems the best approach. This fixes #37 (again). [1]: https://groups.google.com/a/arduino.cc/d/msg/developers/1kkIqIsbuzU/0-abwr1gBQAJ Signed-off-by: Matthijs Kooijman --- src/arduino.cc/builder/builder.go | 2 +- .../print_used_and_not_used_libraries.go | 20 ++++++++++++++----- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/arduino.cc/builder/builder.go b/src/arduino.cc/builder/builder.go index 7e9d08a9..53519555 100644 --- a/src/arduino.cc/builder/builder.go +++ b/src/arduino.cc/builder/builder.go @@ -116,7 +116,7 @@ func (s *Builder) Run(ctx *types.Context) error { mainErr := runCommands(ctx, commands, true) commands = []types.Command{ - &PrintUsedAndNotUsedLibraries{}, + &PrintUsedAndNotUsedLibraries{ SketchError: mainErr != nil }, &PrintUsedLibrariesIfVerbose{}, } diff --git a/src/arduino.cc/builder/print_used_and_not_used_libraries.go b/src/arduino.cc/builder/print_used_and_not_used_libraries.go index 90df1a72..b853ebc2 100644 --- a/src/arduino.cc/builder/print_used_and_not_used_libraries.go +++ b/src/arduino.cc/builder/print_used_and_not_used_libraries.go @@ -36,10 +36,20 @@ import ( "time" ) -type PrintUsedAndNotUsedLibraries struct{} +type PrintUsedAndNotUsedLibraries struct { + // Was there an error while compiling the sketch? + SketchError bool +} func (s *PrintUsedAndNotUsedLibraries) Run(ctx *types.Context) error { - if ctx.DebugLevel < 0 { + var logLevel string + // Print this message as warning when the sketch didn't compile, + // as info when we're verbose and not all otherwise + if s.SketchError { + logLevel = constants.LOG_LEVEL_WARN + } else if ctx.Verbose { + logLevel = constants.LOG_LEVEL_INFO + } else { return nil } @@ -47,10 +57,10 @@ func (s *PrintUsedAndNotUsedLibraries) Run(ctx *types.Context) error { libraryResolutionResults := ctx.LibrariesResolutionResults for header, libResResult := range libraryResolutionResults { - logger.Fprintln(os.Stdout, constants.LOG_LEVEL_WARN, constants.MSG_LIBRARIES_MULTIPLE_LIBS_FOUND_FOR, header) - logger.Fprintln(os.Stdout, constants.LOG_LEVEL_WARN, constants.MSG_LIBRARIES_USED, libResResult.Library.Folder) + logger.Fprintln(os.Stdout, logLevel, constants.MSG_LIBRARIES_MULTIPLE_LIBS_FOUND_FOR, header) + logger.Fprintln(os.Stdout, logLevel, constants.MSG_LIBRARIES_USED, libResResult.Library.Folder) for _, notUsedLibrary := range libResResult.NotUsedLibraries { - logger.Fprintln(os.Stdout, constants.LOG_LEVEL_WARN, constants.MSG_LIBRARIES_NOT_USED, notUsedLibrary.Folder) + logger.Fprintln(os.Stdout, logLevel, constants.MSG_LIBRARIES_NOT_USED, notUsedLibrary.Folder) } }