[close #90] Use relative path for cache keys #92
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fix is targeted at 4.x. Right now the cache is using absolute paths to generate cache keys, this doesn't work if you end up running `rake assets:precompile` in different directories. Say you've moved your project or are running on a different server with a different absolute path. Or say, on every build on Heroku ever. This was reported in rails#59 and rails#90 This commit is a introduces UnloadedAsset class that is responsible for among other things generating consistent cache keys. Previously when running https://gist.github.com/schneems/85f592ba2773761dfcf3 the output would show that the second build took over 38 seconds to build. With this patch applied it takes around 3.38 seconds. Most of this commit is docs, as the current behavior and use of the cache is not really documented, and extremely difficult to follow in the code. All existing behavior is preserved, all tests pass, an extra test was added to ensure that a project built using a previously built directory will be fast.
|
|
schneems
added a commit
that referenced
this pull request
Aug 12, 2015
[close #90] Use relative path for cache keys
schneems
added a commit
that referenced
this pull request
Aug 16, 2015
The patch in #92 was incomplete, it converted all cache keys to use relative paths, but didn't fully remove all absolute paths from cache contents. The test case was accidentally passing due to the copying of files instead of moving files - [x] dependency paths - [x] asset uris - [x] "included" paths (no idea what these are) - [ ] load paths This is a WIP
5 tasks
schneems
added a commit
to schneems/sprockets
that referenced
this pull request
Aug 18, 2015
The patch in rails#92 was incomplete, it converted all cache keys to use relative paths, but didn't fully remove all absolute paths from cache contents. The test case was accidentally passing due to the copying of files instead of moving files - [x] dependency paths - [x] filename - [x] asset uris - [x] "included" paths (no idea what these are) - [x] load paths - [x] update docs - [ ] test broken behavior This is a WIP
schneems
added a commit
that referenced
this pull request
Aug 18, 2015
The patch in #92 was incomplete, it converted all cache keys to use relative paths, but didn't fully remove all absolute paths from cache contents. The test case was accidentally passing since we didn't check to make sure any of the paths from cache were different from the original ones stored. This patch eliminates these absolute paths stored in the cache: - [x] dependency paths - [x] filename - [x] asset uris - [x] "included" paths (no idea what these are) - [x] load paths ### URITar This patch works by introducing a utility class URITar which can "compress" or "expand" a given uri. A "compressed" uri can either be one that is relative to the root, or an absolute path if it is outside of the root. An expanded uri will always be an absolute path. A uri that is relative to the root will be compressed with no beginning slash file://relative/to/root/file.js A uri that is outside of the root will be compressed with a beginning slash file:///absolute/path/to/file.js Even though I'm using "uri" here the URITar class can also operate on file paths without a valid URI scheme name. Like: relative/to/root/file.js and /absolute/path/to/file.js The UnloadedAsset class was moved to it's own file and refactored to use the new URITar class. ## Fix Before putting anything in the cache, we will "compress" all uris and paths so that no absolute paths are in the cache (unless they're not relative to the root which would indicate they're somewhere global e.g. from a gem or shared directory). Upon loading an asset in memory, we "expand" all uris since sprockets relies on absolute paths for just about everything. Almost all the business logic is limited to the loader, so the rest of sprockets has no clue if relative or absolute paths were used to build the asset. We are also compressing the "environment-paths" so that dependencies in different paths will differ. I think this is needed, but the tests don't fail when it's taken out. ATP
schneems
added a commit
that referenced
this pull request
Aug 18, 2015
The patch in #92 was incomplete, it converted all cache keys to use relative paths, but didn't fully remove all absolute paths from cache contents. The test case was accidentally passing since we didn't check to make sure any of the paths from cache were different from the original ones stored. This patch eliminates these absolute paths stored in the cache: - [x] dependency paths - [x] filename - [x] asset uris - [x] "included" paths (no idea what these are) - [x] load paths ### URITar This patch works by introducing a utility class URITar which can "compress" or "expand" a given uri. A "compressed" uri can either be one that is relative to the root, or an absolute path if it is outside of the root. An expanded uri will always be an absolute path. A uri that is relative to the root will be compressed with no beginning slash file://relative/to/root/file.js A uri that is outside of the root will be compressed with a beginning slash file:///absolute/path/to/file.js Even though I'm using "uri" here the URITar class can also operate on file paths without a valid URI scheme name. Like: relative/to/root/file.js and /absolute/path/to/file.js The UnloadedAsset class was moved to it's own file and refactored to use the new URITar class. ## Fix Before putting anything in the cache, we will "compress" all uris and paths so that no absolute paths are in the cache (unless they're not relative to the root which would indicate they're somewhere global e.g. from a gem or shared directory). Upon loading an asset in memory, we "expand" all uris since sprockets relies on absolute paths for just about everything. Almost all the business logic is limited to the loader, so the rest of sprockets has no clue if relative or absolute paths were used to build the asset. We are also compressing the "environment-paths" so that dependencies in different paths will differ. I think this is needed, but the tests don't fail when it's taken out. ATP
schneems
added a commit
to schneems/sprockets
that referenced
this pull request
Aug 18, 2015
The patch in rails#92 was incomplete, it converted all cache keys to use relative paths, but didn't fully remove all absolute paths from cache contents. The test case was accidentally passing since we didn't check to make sure any of the paths from cache were different from the original ones stored. This patch eliminates these absolute paths stored in the cache: - [x] dependency paths - [x] filename - [x] asset uris - [x] "included" paths (no idea what these are) - [x] load paths ### URITar This patch works by introducing a utility class URITar which can "compress" or "expand" a given uri. A "compressed" uri can either be one that is relative to the root, or an absolute path if it is outside of the root. An expanded uri will always be an absolute path. A uri that is relative to the root will be compressed with no beginning slash file://relative/to/root/file.js A uri that is outside of the root will be compressed with a beginning slash file:///absolute/path/to/file.js Even though I'm using "uri" here the URITar class can also operate on file paths without a valid URI scheme name. Like: relative/to/root/file.js and /absolute/path/to/file.js The UnloadedAsset class was moved to it's own file and refactored to use the new URITar class. ## Fix Before putting anything in the cache, we will "compress" all uris and paths so that no absolute paths are in the cache (unless they're not relative to the root which would indicate they're somewhere global e.g. from a gem or shared directory). Upon loading an asset in memory, we "expand" all uris since sprockets relies on absolute paths for just about everything. Almost all the business logic is limited to the loader, so the rest of sprockets has no clue if relative or absolute paths were used to build the asset. We are also compressing the "environment-paths" so that dependencies in different paths will differ. I think this is needed, but the tests don't fail when it's taken out. ATP
schneems
added a commit
to schneems/sprockets
that referenced
this pull request
Aug 19, 2015
The patch in rails#92 was incomplete, it converted all cache keys to use relative paths, but didn't fully remove all absolute paths from cache contents. The test case was accidentally passing since we didn't check to make sure any of the paths from cache were different from the original ones stored. This patch eliminates these absolute paths stored in the cache: - [x] dependency paths - [x] filename - [x] asset uris - [x] "included" paths (no idea what these are) - [x] load paths ### URITar This patch works by introducing a utility class URITar which can "compress" or "expand" a given uri. A "compressed" uri can either be one that is relative to the root, or an absolute path if it is outside of the root. An expanded uri will always be an absolute path. A uri that is relative to the root will be compressed with no beginning slash file://relative/to/root/file.js A uri that is outside of the root will be compressed with a beginning slash file:///absolute/path/to/file.js Even though I'm using "uri" here the URITar class can also operate on file paths without a valid URI scheme name. Like: relative/to/root/file.js and /absolute/path/to/file.js The UnloadedAsset class was moved to it's own file and refactored to use the new URITar class. ## Fix Before putting anything in the cache, we will "compress" all uris and paths so that no absolute paths are in the cache (unless they're not relative to the root which would indicate they're somewhere global e.g. from a gem or shared directory). Upon loading an asset in memory, we "expand" all uris since sprockets relies on absolute paths for just about everything. Almost all the business logic is limited to the loader, so the rest of sprockets has no clue if relative or absolute paths were used to build the asset. We are also compressing the "environment-paths" so that dependencies in different paths will differ. I think this is needed, but the tests don't fail when it's taken out. ATP
schneems
added a commit
that referenced
this pull request
Aug 19, 2015
Targeted at sprockets 4.x The patch in #92 was incomplete, it converted all cache keys to use relative paths, but didn't fully remove all absolute paths from cache contents. The test case was accidentally passing since we didn't check to make sure any of the paths from cache were different from the original ones stored. This patch eliminates these absolute paths stored in the cache: - [x] dependency paths - [x] filename - [x] asset uris - [x] "included" paths (no idea what these are) - [x] load paths This patch works by introducing a utility class URITar which can "compress" or "expand" a given uri. A "compressed" uri can either be one that is relative to the root, or an absolute path if it is outside of the root. An expanded uri will always be an absolute path. A uri that is relative to the root will be compressed with no beginning slash file://relative/to/root/file.js A uri that is outside of the root will be compressed with a beginning slash file:///absolute/path/to/file.js Even though I'm using "uri" here the URITar class can also operate on file paths without a valid URI scheme name. Like: relative/to/root/file.js and /absolute/path/to/file.js The UnloadedAsset class was moved to it's own file and refactored to use the new URITar class. Before putting anything in the cache, we will "compress" all uris and paths so that no absolute paths are in the cache (unless they're not relative to the root which would indicate they're somewhere global e.g. from a gem or shared directory). Upon loading an asset in memory, we "expand" all uris since sprockets relies on absolute paths for just about everything. Almost all the business logic is limited to the loader, so the rest of sprockets has no clue if relative or absolute paths were used to build the asset. We are also compressing the "environment-paths" so that dependencies in different paths will differ. I think this is needed, but the tests don't fail when it's taken out. ATP
5 tasks
schneems
added a commit
that referenced
this pull request
Aug 20, 2015
Previous pull requests (#92 & #101) did not properly "compress": `links`, `stubbed`, or `required` keys in the metadata. Also external processors can put arbitrary in the metadata hash, we now have a convention where anything that ends in `_dependencies` such as `sass_dependencies` will also be compressed. This change properly compresses and expand uris. We also need to expand uris in the `fetch_asset_from_dependency_cache` before they are resolved to generate a digest. We must also make sure to compress them before they are stored back in the cache. There might be a better place to put this logic somewhere in the future, but for now it makes sense to put both expansion and compression in the same method, though this does mean we have to "compress" the dependencies twice. Tests are written that will fail without this patch and pass with it. The "stubbed" and "required" sets in the metadata are not directly exposed, we must use private methods to observe them. I think it is better to do this for now, I believe this file has some refactor potential in the future, but don't want to mix bug fixes with refactoring.
schneems
added a commit
that referenced
this pull request
Aug 20, 2015
Previous pull requests (#92 & #101) did not properly "compress": `links`, `stubbed`, or `required` keys in the metadata. Also external processors can put arbitrary in the metadata hash, we now have a convention where anything that ends in `_dependencies` such as `sass_dependencies` will also be compressed. This change properly compresses and expand uris. We also need to expand uris in the `fetch_asset_from_dependency_cache` before they are resolved to generate a digest. We must also make sure to compress them before they are stored back in the cache. There might be a better place to put this logic somewhere in the future, but for now it makes sense to put both expansion and compression in the same method, though this does mean we have to "compress" the dependencies twice. Tests are written that will fail without this patch and pass with it. The "stubbed" and "required" sets in the metadata are not directly exposed, we must use private methods to observe them. I think it is better to do this for now, I believe this file has some refactor potential in the future, but don't want to mix bug fixes with refactoring.
schneems
added a commit
that referenced
this pull request
Aug 21, 2015
Previous pull requests (#92 & #101) did not properly "compress": `links`, `stubbed`, or `required` keys in the metadata. Also external processors can put arbitrary in the metadata hash, we now have a convention where anything that ends in `_dependencies` such as `sass_dependencies` will also be compressed. This change properly compresses and expand uris. We also need to expand uris in the `fetch_asset_from_dependency_cache` before they are resolved to generate a digest. We must also make sure to compress them before they are stored back in the cache. There might be a better place to put this logic somewhere in the future, but for now it makes sense to put both expansion and compression in the same method, though this does mean we have to "compress" the dependencies twice. Tests are written that will fail without this patch and pass with it. The "stubbed" and "required" sets in the metadata are not directly exposed, we must use private methods to observe them. I think it is better to do this for now, I believe this file has some refactor potential in the future, but don't want to mix bug fixes with refactoring.
schneems
added a commit
that referenced
this pull request
Aug 26, 2015
Previous pull requests (#92 & #101) did not properly "compress": `links`, `stubbed`, or `required` keys in the metadata. Also external processors can put arbitrary in the metadata hash, we now have a convention where anything that ends in `_dependencies` such as `sass_dependencies` will also be compressed. This change properly compresses and expand uris. We also need to expand uris in the `fetch_asset_from_dependency_cache` before they are resolved to generate a digest. We must also make sure to compress them before they are stored back in the cache. There might be a better place to put this logic somewhere in the future, but for now it makes sense to put both expansion and compression in the same method, though this does mean we have to "compress" the dependencies twice. Tests are written that will fail without this patch and pass with it. The "stubbed" and "required" sets in the metadata are not directly exposed, we must use private methods to observe them. I think it is better to do this for now, I believe this file has some refactor potential in the future, but don't want to mix bug fixes with refactoring.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fix is targeted at 4.x.
Right now the cache is using absolute paths to generate cache keys, this doesn't work if you end up running
rake assets:precompilein different directories. Say you've moved your project or are running on a different server with a different absolute path. Or say, on every build on Heroku ever. This was reported in #59 and #90This commit is a introduces UnloadedAsset class that is responsible for among other things generating consistent cache keys.
Previously when running https://gist.github.com/schneems/85f592ba2773761dfcf3 the output would show that the second build took over 38 seconds to build. With this patch applied it takes around 3.38 seconds.
Most of this commit is docs, as the current behavior and use of the cache is not really documented, and extremely difficult to follow in the code. All existing behavior is preserved, all tests pass, an extra test was added to ensure that a project built using a previously built directory will be fast.