Skip to content

Improve core archive info #288

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.

Sign up for GitHub

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 3 commits into from
Sep 7, 2018
Merged

Improve core archive info #288

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ab41198
Avoid very long core archive names
facchinm Sep 4, 2018
2242b5c
Improve error reporting on core archive creation
facchinm Sep 4, 2018
d2a559f
Complain core maintainers only when appropriate
cmaglie Sep 7, 2018
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
Prev Previous commit
Complain core maintainers only when appropriate
  • Loading branch information
@cmaglie
cmaglie committed Sep 7, 2018
commit d2a559f3043f3ffb86014c8116884959f0afe8e7
1 change: 1 addition & 0 deletions constants/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ const LOG_LEVEL_INFO = "info"
const LOG_LEVEL_WARN = "warn"
const MSG_ARCH_FOLDER_NOT_SUPPORTED = "'arch' folder is no longer supported! See http://goo.gl/gfFJzU for more information"
const MSG_ARCHIVING_CORE_CACHE = "Archiving built core (caching) in: {0}"
const MSG_ERROR_ARCHIVING_CORE_CACHE = "Error archiving built core (caching) in {0}: {1}"
const MSG_CORE_CACHE_UNAVAILABLE = "Unable to cache built core, please tell {0} maintainers to follow http://goo.gl/QdCUjo"
const MSG_BOARD_UNKNOWN = "Board {0} (platform {1}, package {2}) is unknown"
const MSG_BOOTLOADER_FILE_MISSING = "Bootloader file specified but missing: {0}"
Expand Down
5 changes: 4 additions & 1 deletion phases/core_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
package phases

import (
"os"
"path/filepath"

"github.com/arduino/arduino-builder/builder_utils"
Expand Down Expand Up @@ -128,8 +129,10 @@ func compileCore(ctx *types.Context, buildPath string, buildCachePath string, bu
if ctx.Verbose {
if err == nil {
logger.Println(constants.LOG_LEVEL_INFO, constants.MSG_ARCHIVING_CORE_CACHE, targetArchivedCore)
} else {
} else if os.IsNotExist(err) {
logger.Println(constants.LOG_LEVEL_INFO, constants.MSG_CORE_CACHE_UNAVAILABLE, ctx.ActualPlatform.PlatformId)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This blames any error while copying the file on the core maintainers. I suppose the intended case is where the core does not use the proper .a filename, so the copy cannot find it? If so, then perhaps a separate check should be made to see if the .a filename exists and give this error if it does not? If the file does exist, but the copy fails, a new error message should probably be shown?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd also add arduino.ar as a possible target but this clashes badly with specifications at http://goo.gl/QdCUjo .
I think that adding this check will make life terrible for maintainers of "incompatible" cores for a bit but will enormously speedup the adoption of proper naming (esp8266 just merged it in mainline esp8266/Arduino@85e6809#diff-d638a47d1ca0e2a5585fb34c7d7fb0f9L95 )

Copy link
Collaborator

Choose a reason for hiding this comment

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

Terrible in what way? Because users will start pestering them? Or because their incompatible cores will break or become slower (I can't see why or how)?

Copy link
Member Author

Choose a reason for hiding this comment

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

The first one 🙂

} else {
logger.Println(constants.LOG_LEVEL_INFO, constants.MSG_ERROR_ARCHIVING_CORE_CACHE, targetArchivedCore, err)
}
}
}
Expand Down