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

Replace fixtures.tar.gz with regular files #56

Merged
merged 2 commits into from
Jun 19, 2017

Conversation

ideaship
Copy link
Contributor

@ideaship ideaship commented Jun 14, 2017

In order to keep the fixtures in a form that can easily be reviewed,
this changeset puts the files from fixtures.tar.gz back into the repo as
regular files (in a separate directory, fixtures.src). In order to
prevent filenames that prevent checkouts on Windows, colons are encoded
as _@colon@_ and copied to the correct location by make.

Alternatives: files could be renamed rather than copied (using -depth
in the find command to rename files before their parent directories).
However, doing so would remove files from a location where git expects
them.

In order to keep the fixtures in a form that can easily be reviewed,
this changeset puts the files from fixtures.tar.gz back into the repo as
regular files (in a separate directory, fixtures.src). In order to
prevent filenames that prevent checkouts on Windows, colons are encoded
as "_@colon@_" and copied to the correct location by make.

Alternatives: files could be renamed rather than copied (using -depth
in the find command to rename files before their parent directories).
However, doing so would remove files from a location where git expects
them.
Makefile Outdated
@@ -8,11 +8,17 @@ lint:
go get github.com/golang/lint/golint
golint *.go

test: sysfs/fixtures/.unpacked
test: sysfs/fixtures/.copied
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using a marker file, which can get easily out of sync, I'd like to use each @colon@ file as dependency here. @matthiasr we do something similar in a couple of SoundCloud projects, do you have some time to use your make skills to apply that here?

I'll try to port it this weekend otherwise.

Choose a reason for hiding this comment

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

Working on it. The straightforward way will require GNU make, is that acceptable?

Copy link

@matthiasr matthiasr Jun 17, 2017

Choose a reason for hiding this comment

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

Turns out, I can't make it work. Make does not handle filenames with characters well, so specifying the target names breaks it completely. Not specifying the target names means we end up in the same out-of-sync situation with the added downside of redoing all the work all the time.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I guess then I'd vote to remove the marker and rename them every time. Otherwise developers can run in staleness issues when others make changes to the fixtures directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a renaming routine quite similar to the copy code in this PR. Obviously, this means that after a make test, every renamed file will show up as missing for git. Also, giving up the marker makes the test process slower. We have to walk the file try every time to check for files that need renaming. And a file removed from git won't get the renamed version removed automatically. For that, we'd have to rebuild the directory from scratch every time (also doable if that is your preference).

Copy link
Member

Choose a reason for hiding this comment

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

The problem is, the marker needs to be somehow dependent on the files in source code. Otherwise the copy command will never be executed again. That's what I asked @matthiasr to help with, but apparently it's not possible due to Make's lack of colon support in filenames.

I'd prefer slower test execution over stale fixture files. I'd like to prevent developers from unnecessary debug sessions when they forgot to remove the marker file manually to trigger an update.

@grobie
Copy link
Member

grobie commented Jun 17, 2017 via email

@matthiasr
Copy link

Is it available on osx?

Yes, it's the default there. It's not on other BSDs, but can be installed.

@matthiasr
Copy link

matthiasr commented Jun 19, 2017 via email

In order to avoid any test on a stale fixtures tree, remove and rebuild
the directory every time.
@grobie
Copy link
Member

grobie commented Jun 19, 2017

Thank you @ideaship!

@grobie grobie merged commit b9955ae into prometheus:master Jun 19, 2017
@ideaship
Copy link
Contributor Author

Sigh @ #57. I need to find a Windows box for testing.

grobie added a commit that referenced this pull request Jun 20, 2017
The generated pathnames are too long for some filenames, including NTFS
in its default configuration. We've considered using other separators,
but deemed shorter ones to easy to break something.

This reverts commit b9955ae, reversing
changes made to a3bfc74.

Fixes #57.
@ideaship ideaship deleted the copy_fixtures_02 branch July 21, 2017 14:22
remijouannet pushed a commit to remijouannet/procfs that referenced this pull request Oct 20, 2022
Replace fixtures.tar.gz with regular files
remijouannet pushed a commit to remijouannet/procfs that referenced this pull request Oct 20, 2022
The generated pathnames are too long for some filenames, including NTFS
in its default configuration. We've considered using other separators,
but deemed shorter ones to easy to break something.

This reverts commit b9955ae, reversing
changes made to a3bfc74.

Fixes prometheus#57.
bobrik pushed a commit to bobrik/procfs that referenced this pull request Jan 14, 2023
…-root

Introduce Process::new_with_root() constructor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants