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

fix: race condition and remove unused code #273

Merged
merged 8 commits into from
Dec 30, 2020
Merged
Show file tree
Hide file tree
Changes from 7 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
81 changes: 23 additions & 58 deletions files/files.go
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
package files

import (
"fmt"
"os"
"path/filepath"
"sort"
"time"

"github.com/goreleaser/fileglob"
"gopkg.in/yaml.v3"

"github.com/goreleaser/nfpm/v2/internal/glob"
)

Expand All @@ -34,44 +31,6 @@ type ContentFileInfo struct {
// Contents list of Content to process.
type Contents []*Content

type simpleContents map[string]string

func (c *Contents) UnmarshalYAML(value *yaml.Node) (err error) {
// nolint:exhaustive
// we do not care about `AliasNode`, `DocumentNode`
switch value.Kind {
case yaml.SequenceNode:
type tmpContents Contents
var tmp tmpContents
if err = value.Decode(&tmp); err != nil {
return err
}
*c = Contents(tmp)
case yaml.MappingNode:
var tmp simpleContents
if err = value.Decode(&tmp); err != nil {
return err
}
for src, dst := range tmp {
*c = append(*c, &Content{
Source: src,
Destination: dst,
})
}
case yaml.ScalarNode:
// TODO: implement issue-43 here and remove the fallthrough
// nolint:gocritic
// ignoring `emptyFallthrough: remove empty case containing only fallthrough to default case`
fallthrough
default:
// nolint:goerr113
// this is temporary so we do not need the a static error
return fmt.Errorf("not implemented")
}

return nil
}

func (c Contents) Len() int {
return len(c)
}
Expand All @@ -91,25 +50,33 @@ func (c Contents) Less(i, j int) bool {
return a.Destination < b.Destination
}

func (c *Content) WithFileInfoDefaults() {
if c.FileInfo == nil {
c.FileInfo = &ContentFileInfo{}
func (c *Content) WithFileInfoDefaults() *Content {
Copy link
Member Author

Choose a reason for hiding this comment

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

this method could be called by multiple threads at once, therefore we probably need to modify and return a copy instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

var cc = &Content{
Source: c.Source,
Destination: c.Destination,
Type: c.Type,
Packager: c.Packager,
FileInfo: c.FileInfo,
}
if cc.FileInfo == nil {
cc.FileInfo = &ContentFileInfo{}
}
if c.FileInfo.Owner == "" {
c.FileInfo.Owner = "root"
if cc.FileInfo.Owner == "" {
cc.FileInfo.Owner = "root"
}
if c.FileInfo.Group == "" {
c.FileInfo.Group = "root"
if cc.FileInfo.Group == "" {
cc.FileInfo.Group = "root"
}
info, err := os.Stat(c.Source)
info, err := os.Stat(cc.Source)
if err == nil {
c.FileInfo.MTime = info.ModTime()
c.FileInfo.Mode = info.Mode()
c.FileInfo.Size = info.Size()
cc.FileInfo.MTime = info.ModTime()
cc.FileInfo.Mode = info.Mode()
cc.FileInfo.Size = info.Size()
}
if c.FileInfo.MTime.IsZero() {
c.FileInfo.MTime = time.Now().UTC()
if cc.FileInfo.MTime.IsZero() {
cc.FileInfo.MTime = time.Now().UTC()
}
return cc
}

// Name to part of the os.FileInfo interface
Expand Down Expand Up @@ -153,8 +120,7 @@ func ExpandContentGlobs(filesSrcDstMap Contents, disableGlobbing bool) (files Co
switch f.Type {
case "ghost", "symlink":
// Ghost and symlink files need to be in the list, but dont glob them because they do not really exist
f.WithFileInfoDefaults()
files = append(files, f)
files = append(files, f.WithFileInfoDefaults())
continue
}

Expand All @@ -181,8 +147,7 @@ func appendGlobbedFiles(globbed map[string]string, origFile *Content, incFiles C
FileInfo: origFile.FileInfo,
Packager: origFile.Packager,
}
newFile.WithFileInfoDefaults()
files = append(files, newFile)
files = append(files, newFile.WithFileInfoDefaults())
}

return files
Expand Down
36 changes: 15 additions & 21 deletions files/files_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package files_test

import (
"strings"
"sync"
"testing"

"github.com/stretchr/testify/assert"
Expand All @@ -24,9 +25,9 @@ contents:
- src: a
dst: b
type: "config|noreplace"
packager: "rpm"
file_info:
mode: 0644
packager: "rpm"
mtime: 2008-01-02T15:04:05Z
`))
dec.KnownFields(true)
Expand All @@ -40,31 +41,24 @@ contents:
}
}

func TestMapperDecode(t *testing.T) {
func TestRace(t *testing.T) {
var config testStruct
dec := yaml.NewDecoder(strings.NewReader(`---
contents:
a: b
a2: b2
- src: a
dst: b
type: symlink
`))
dec.KnownFields(true)
err := dec.Decode(&config)
require.NoError(t, err)
assert.Len(t, config.Contents, 2)
for _, f := range config.Contents {
t.Logf("%+#v\n", f)
assert.Equal(t, f.Packager, "")
assert.Equal(t, f.Type, "")
var wg sync.WaitGroup
for i := 0; i < 10; i++ {
wg.Add(1)
go func() {
defer wg.Done()
_, err := files.ExpandContentGlobs(config.Contents, false)
require.NoError(t, err)
}()
}
}

func TestStringDecode(t *testing.T) {
var config testStruct
dec := yaml.NewDecoder(strings.NewReader(`---
contents: /path/to/a/tgz
`))
dec.KnownFields(true)
err := dec.Decode(&config)
require.Error(t, err)
assert.Equal(t, err.Error(), "not implemented")
wg.Wait()
}
9 changes: 7 additions & 2 deletions nfpm.go
Original file line number Diff line number Diff line change
Expand Up @@ -305,8 +305,13 @@ func Validate(info *Info) (err error) {
return ErrFieldEmpty{"version"}
}

info.Contents, err = files.ExpandContentGlobs(info.Contents, info.DisableGlobbing)
return err
contents, err := files.ExpandContentGlobs(info.Contents, info.DisableGlobbing)
if err != nil {
return err
}

info.Contents = contents
return nil
}

// WithDefaults set some sane defaults into the given Info.
Expand Down
6 changes: 3 additions & 3 deletions testdata/contents.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,18 @@ contents:
- src: "./testdata/whatever2.conf"
dst: "/etc/rpm/whatever.conf"
type: "config|noreplace"
packager: "rpm"
file_info:
mode: 0644
packager: "rpm"
- src: "./testdata/whatever2.conf"
dst: "/etc/apk/whatever.conf"
type: "config|noreplace"
packager: "apk"
file_info:
mode: 0644
packager: "apk"
- src: "./testdata/whatever2.conf"
dst: "/etc/deb/whatever.conf"
type: "config|noreplace"
packager: "deb"
file_info:
mode: 0644
packager: "deb"
67 changes: 34 additions & 33 deletions www/docs/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,62 +85,63 @@ suggests:
# Packages it conflicts with. (overridable)
conflicts:
- mercurial

# Contents to add to the package
# This can be binaries or any other files.
contents:
# Basic file that applies to all packagers
# Basic file that applies to all packagers
- src: path/to/local/foo
dst: /usr/local/bin/foo
# Simple config file

# Simple config file
- src: path/to/local/foo.conf
dst: /etc/foo.conf
type: config

# Simple symlink
- src: /sbin/foo # link name
dst: /usr/local/bin/foo # real location
type: "symlink"

# Corresponds to %config(noreplace) if the packager is rpm, otherwise it is just a config file

# Simple symlink
# Will result in `ln -s /sbin/foo /usr/local/bin/foo`.
- src: /sbin/foo
dst: /usr/local/bin/foo
type: symlink

# Corresponds to `%config(noreplace)` if the packager is rpm, otherwise it is just a config file
- src: path/to/local/bar.conf
dst: /etc/bar.conf
type: "config|noreplace"

# These files are not actually present in the package, but the file names
# are added to the package header. From the RPM directives documentation:
#
# "There are times when a file should be owned by the package but not
# installed - log files and state files are good examples of cases you might
# desire this to happen."
#
# "The way to achieve this, is to use the %ghost directive. By adding this
# directive to the line containing a file, RPM will know about the ghosted
# file, but will not add it to the package."
#
# For non rpm packages ghost files are ignored at this time.
type: config|noreplace

# These files are not actually present in the package, but the file names
# are added to the package header. From the RPM directives documentation:
#
# "There are times when a file should be owned by the package but not
# installed - log files and state files are good examples of cases you might
# desire this to happen."
#
# "The way to achieve this, is to use the %ghost directive. By adding this
# directive to the line containing a file, RPM will know about the ghosted
# file, but will not add it to the package."
#
# For non rpm packages ghost files are ignored at this time.
- dst: /etc/casper.conf
type: ghost
- dst: /var/log/boo.log
type: ghost
# You can user the packager field to add files that are unique to a specific packager

# You can user the packager field to add files that are unique to a specific packager
- src: path/to/rpm/file.conf
dst: /etc/file.conf
type: "config|noreplace"
type: config|noreplace
packager: rpm
- src: path/to/deb/file.conf
dst: /etc/file.conf
type: "config|noreplace"
type: config|noreplace
packager: deb
- src: path/to/apk/file.conf
dst: /etc/file.conf
type: "config|noreplace"
type: config|noreplace
packager: apk
# Sometimes it is important to be able to set the mtime, mode, owner, or group for a file
# that differs from what is on the local build system at build time.

# Sometimes it is important to be able to set the mtime, mode, owner, or group for a file
# that differs from what is on the local build system at build time.
- src: path/to/foo
dst: /usr/local/foo
file_info:
Expand Down